Re: [PATCH 1/3] attr: use consistent sgid stripping checks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Oct 06, 2022 at 08:28:51AM +1100, Dave Chinner wrote:
> On Wed, Oct 05, 2022 at 05:14:31PM +0200, Christian Brauner wrote:
> > diff --git a/fs/inode.c b/fs/inode.c
> > index ba1de23c13c1..4f3257f5ed7a 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -1949,26 +1949,44 @@ void touch_atime(const struct path *path)
> >  }
> >  EXPORT_SYMBOL(touch_atime);
> >  
> > -/*
> > - * The logic we want is
> > +/**
> > + * should_remove_sgid - determine whether the setgid bit needs to be removed
> > + * @mnt_userns:	User namespace of the mount the inode was created from
> > + * @inode: inode to check
> > + *
> > + * This function determines whether the setgid bit needs to be removed.
> > + * We retain backwards compatibility where we require the setgid bit to be
> > + * removed unconditionally if S_IXGRP is set. Otherwise we have the exact same
> > + * requirements as setattr_prepare() and setattr_copy().
> >   *
> > - *	if suid or (sgid and xgrp)
> > - *		remove privs
> > + * Return: true if setgit bit needs to be removed, false otherwise.
> >   */
> > -int should_remove_suid(struct dentry *dentry)
> > +static bool should_remove_sgid(struct user_namespace *mnt_userns,
> > +			       struct inode *inode)
> > +{
> > +	umode_t mode = inode->i_mode;
> > +
> > +	if (unlikely(mode & S_ISGID)) {
> > +		if ((mode & S_IXGRP) ||
> > +		    (!vfsgid_in_group_p(i_gid_into_vfsgid(mnt_userns, inode)) &&
> > +		     !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)))
> > +			return true;
> > +	}
> > +
> > +	return false;
> 
> I find this sort of convoluted logic much easier to follow when it's
> written as a stacked set of single comparisons like so:
> 
> 	if (!(mode & S_ISGID))
> 		return false;
> 	if (mode & S_IXGRP)
> 		return true;
> 	if (vfsgid_in_group_p(i_gid_into_vfsgid(mnt_userns, inode))
> 		return false;
> 	if (capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID)
> 		return false;
> 	return true;

Good idea, I'll fix that up in tree.

Thanks!
Christian



[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux