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@xxxxxxxxxxxxx>
>> Signed-off-by: Yang Xu<xuyang2018.jy@xxxxxxxxxxx>
>> ---
>> 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
>>
