| Message ID | 1648461389-2225-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/3] vfs: Add inode_sgid_strip() api | expand |
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> > --- I would've personally gone for returning umode_t but this work for me too. Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
On Tue, Mar 29, 2022 at 12:45:16PM +0200, Christian Brauner 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> > > --- > > I would've personally gone for returning umode_t but this work for me > too. Agreed, that's a much nicer API for this function - it makes it clear that it can modifying the mode that is passed in. Cheers, Dave.
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 >
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 >>
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; +} +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
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(+)