Re: [PATCH v3 2/5] attr: add setattr_should_drop_sgid()

[Amir Goldstein <amir73il@gmail.com>]

On Mon, Oct 17, 2022 at 1:06 PM Christian Brauner <brauner@kernel.org> 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
> setattr_should_drop_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@kernel.org>

Some nits, but otherwise

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>
> Notes:
>     /* v2 */
>     Dave Chinner <dchinner@redhat.com>:
>     - Use easier to follow logic in the new helper.
>
>     /* v3 */
>     Amir Goldstein <amir73il@gmail.com>:
>     - Rename helper from should_remove_sgid() to setattr_should_drop_sgid() to
>       better indicate its semantics.
>     - Return setattr_should_drop_sgid() directly now that it returns ATTR_KILL_SGID
>       instead of a boolean.
>
>  fs/attr.c     | 26 ++++++++++++++++++++++++++
>  fs/internal.h |  2 ++
>  2 files changed, 28 insertions(+)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 8bc2edd6bd3c..3d03ceb332e5 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -39,6 +39,32 @@ static int setattr_drop_sgid(struct user_namespace *mnt_userns,
>         return ATTR_KILL_SGID;
>  }
>
> +/**
> + * setattr_should_drop_sgid - determine whether the setgid bit needs to be
> + *                            removed
> + * @mnt_userns:        user namespace of the mount @inode was found 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 setattr_should_drop_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;
> +       return setattr_drop_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 6f0386b34fae..988e123d3885 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -234,3 +234,5 @@ int do_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>                 struct xattr_ctx *ctx);
>
>  ssize_t __kernel_write_iter(struct file *file, struct iov_iter *from, loff_t *pos);
> +int setattr_should_drop_sgid(struct user_namespace *mnt_userns,
> +                            const struct inode *inode);

Please add a section:
/*
 * fs/attr.c:
 */

Might as well as the missing section for read_write.c
above __kernel_write_iter() while at it.

Thanks,
Amir.