Re: [PATCH v2 2/5] attr: add should_remove_sgid()

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

 



On Tue, Oct 11, 2022 at 11:18:22AM +0300, Amir Goldstein wrote:
> On Fri, Oct 7, 2022 at 5:06 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
> > The current setgid stripping logic during write and ownership change
> > operations is inconsistent and strewn over multiple places. In order to
> > consolidate it and make more consistent we'll add a new helper
> > should_remove_sgid(). The function retains the old behavior where we
> > remove the S_ISGID bit unconditionally when S_IXGRP is set but also when
> > it isn't set and the caller is neither in the group of the inode nor
> > privileged over the inode.
> >
> > We will use this helper both in write operation permission removal such
> > as file_remove_privs() as well as in ownership change operations.
> >
> > Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
> 
> Looks good.
> Some suggestions below.
> 
> > ---
> >
> > Notes:
> >     /* v2 */
> >     Dave Chinner <dchinner@xxxxxxxxxx>:
> >     - Use easier to follow logic in the new helper.
> >
> >  fs/attr.c     | 27 +++++++++++++++++++++++++++
> >  fs/internal.h |  2 ++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/fs/attr.c b/fs/attr.c
> > index b1cff6f5b715..d0bb1dae425e 100644
> > --- a/fs/attr.c
> > +++ b/fs/attr.c
> > @@ -39,6 +39,33 @@ static bool setattr_drop_sgid(struct user_namespace *mnt_userns,
> >         return true;
> >  }
> >
> > +/**
> > + * 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 and require setgid bit to be removed
> > + * unconditionally if S_IXGRP is set. Otherwise we have the exact same
> > + * requirements as setattr_prepare() and setattr_copy().
> > + *
> > + * Return: ATTR_KILL_SGID if setgid bit needs to be removed, 0 otherwise.
> > + */
> > +int should_remove_sgid(struct user_namespace *mnt_userns,
> > +                      const struct inode *inode)
> > +{
> > +       umode_t mode = inode->i_mode;
> > +
> > +       if (!(mode & S_ISGID))
> > +               return 0;
> > +       if (mode & S_IXGRP)
> > +               return ATTR_KILL_SGID;
> > +       if (setattr_drop_sgid(mnt_userns, inode,
> > +                             i_gid_into_vfsgid(mnt_userns, inode)))
> > +               return ATTR_KILL_SGID;
> > +       return 0;
> 
> If you take my suggestion from patch 1/5, that would become:
> 
>     return setattr_should_remove_sgid(mnt_userns, inode,
>                                  i_gid_into_vfsgid(mnt_userns, inode));
> 
> > +}
> > +
> >  /**
> >   * chown_ok - verify permissions to chown inode
> >   * @mnt_userns:        user namespace of the mount @inode was found from
> > diff --git a/fs/internal.h b/fs/internal.h
> > index 87e96b9024ce..9d165ab65a2a 100644
> > --- a/fs/internal.h
> > +++ b/fs/internal.h
> > @@ -221,3 +221,5 @@ ssize_t do_getxattr(struct user_namespace *mnt_userns,
> >  int setxattr_copy(const char __user *name, struct xattr_ctx *ctx);
> >  int do_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
> >                 struct xattr_ctx *ctx);
> > +int should_remove_sgid(struct user_namespace *mnt_userns,
> > +                      const struct inode *inode);
> 
> I realize that you placed this helper in attr.c to make
> setattr_drop_sgid() static, but IMO the code will be clearer to readers
> if all the family of suig/sgid stripping helpers were clustered together
> in inode.c where it will be easier to get the high level view.

I think then we should rather move all those helpers into attr.c. After
all it's is setting/returning iattr flags. Then they can be called from
inode.c



[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