Re: [PATCH v2 1/5] attr: add setattr_drop_sgid()

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

 



On Fri, Oct 7, 2022 at 5:06 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> In setattr_{copy,prepare}() we need to perform the same permission
> checks to determine whether we need to drop the setgid bit or not.
> Instead of open-coding it twice add a simple helper the encapsulates the
> logic. We will reuse this helpers to make dropping the setgid bit during
> write operations more consistent in a follow up patch.
>
> Signed-off-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>

Looks good.
Some suggestions below - not  a must.

Thanks,
Amir.

> ---
>
> Notes:
>     /* v2 */
>     patch added
>
>  fs/attr.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 1552a5f23d6b..b1cff6f5b715 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -18,6 +18,27 @@
>  #include <linux/evm.h>
>  #include <linux/ima.h>
>
> +/**
> + * setattr_drop_sgid - check generic setgid permissions

Helper name sounds like a directive, where it should sound like a question.
e.g. setattr_should_remove_sgid()

> + * @mnt_userns:        User namespace of the mount the inode was created from
> + * @inode: inode to check
> + * @vfsgid: the new/current vfsgid of @inode
> + *
> + * This function determines whether the setgid bit needs to be removed because
> + * the caller lacks privileges over the inode.
> + *
> + * Return: true if the setgid bit needs to be removed, false if not.

You may want to consider matching the return value to that of
should_remove_sgid(), that is 0 or ATTR_KILL_SGID to make all the family of
those helpers behave similarly.

> + */
> +static bool setattr_drop_sgid(struct user_namespace *mnt_userns,
> +                             const struct inode *inode, vfsgid_t vfsgid)
> +{
> +       if (vfsgid_in_group_p(vfsgid))
> +               return false;
> +       if (capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
> +               return false;
> +       return true;
> +}
> +
>  /**
>   * chown_ok - verify permissions to chown inode
>   * @mnt_userns:        user namespace of the mount @inode was found from
> @@ -140,8 +161,7 @@ int setattr_prepare(struct user_namespace *mnt_userns, struct dentry *dentry,
>                         vfsgid = i_gid_into_vfsgid(mnt_userns, inode);
>
>                 /* Also check the setgid bit! */
> -               if (!vfsgid_in_group_p(vfsgid) &&
> -                   !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
> +               if (setattr_drop_sgid(mnt_userns, inode, vfsgid))
>                         attr->ia_mode &= ~S_ISGID;
>         }
>
> @@ -251,9 +271,8 @@ void setattr_copy(struct user_namespace *mnt_userns, struct inode *inode,
>                 inode->i_ctime = attr->ia_ctime;
>         if (ia_valid & ATTR_MODE) {
>                 umode_t mode = attr->ia_mode;
> -               vfsgid_t vfsgid = i_gid_into_vfsgid(mnt_userns, inode);
> -               if (!vfsgid_in_group_p(vfsgid) &&
> -                   !capable_wrt_inode_uidgid(mnt_userns, inode, CAP_FSETID))
> +               if (setattr_drop_sgid(mnt_userns, inode,
> +                                     i_gid_into_vfsgid(mnt_userns, inode)))
>                         mode &= ~S_ISGID;
>                 inode->i_mode = mode;
>         }
> --
> 2.34.1
>



[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