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

[Dave Chinner <david@xxxxxxxxxxxxx>]

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;

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx