Re: [PATCH v1 1/3] vfs: Add inode_sgid_strip() api

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Mar 28, 2022 at 05:56:27PM +0800, Yang Xu wrote:
> inode_sgid_strip() function is used to strip S_ISGID mode
> when creat/open/mknod file.
> 
> Suggested-by: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---
>  fs/inode.c         | 12 ++++++++++++
>  include/linux/fs.h |  3 +++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 63324df6fa27..1f964e7f9698 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2405,3 +2405,15 @@ struct timespec64 current_time(struct inode *inode)
>  	return timestamp_truncate(now, inode);
>  }
>  EXPORT_SYMBOL(current_time);
> +
> +void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
> +		      umode_t *mode)
> +{
> +	if ((dir && dir->i_mode & S_ISGID) &&
> +		(*mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
> +		!S_ISDIR(*mode) &&
> +		!in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
> +		!capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
> +		*mode &= ~S_ISGID;

A couple of style nits here:

The secondary if test clauses have the same indentation level as the
code that actually gets executed, which makes this harder to scan
visually.

	if ((dir && dir->i_mode & S_ISGID) &&
	    (*mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
	    !S_ISDIR(*mode) &&
	    !in_group_p(i_gid_into_mnt(mnt_userns, dir)) &&
	    !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID))
		*mode &= ~S_ISGID;

Alternately, you could use inverse logic to bail out early:

void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
		      umode_t *mode)
{
	if (!dir || !(dir->i_mode & S_ISGID))
		return;
	if (S_ISDIR(*mode))
		return;
	if (in_group_p(...))
		return;
	if (capable_wrt_inode_uidgid(...))
		return;

	*mode &= ~S_ISGID;
}

Though I suppose that /is/ much longer.

The bigger thing here is that I'd like to see this patch hoist the ISGID
stripping code out of init_inode_owner so that it's easier to verify
that the new helper does exactly the same thing as the old code.  The
second patch would then add callsites around the VFS as necessary to
prevent this problem from happening again.

--D

> +}
> +EXPORT_SYMBOL(inode_sgid_strip);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e2d892b201b0..639c830ad797 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1921,6 +1921,9 @@ extern long compat_ptr_ioctl(struct file *file, unsigned int cmd,
>  void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode,
>  		      const struct inode *dir, umode_t mode);
>  extern bool may_open_dev(const struct path *path);
> +void inode_sgid_strip(struct user_namespace *mnt_userns, struct inode *dir,
> +		      umode_t *mode);
> +
>  
>  /*
>   * This is the "filldir" function type, used by readdir() to let
> -- 
> 2.27.0
>