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
