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

["xuyang2018.jy@fujitsu.com" <xuyang2018.jy@fujitsu.com>]

on 2022/3/31 0:34, Darrick J. Wong wrote:
> 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.

Sounds reasonable. Will do it in v2.

Best Regards
Yang Xu
>
> --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
>>