| Message ID | 1657779088-2242-1-git-send-email-xuyang2018.jy@fujitsu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v10,1/4] fs: add mode_strip_sgid() helper | expand |
On Thu, Jul 14, 2022 at 02:11:25PM +0800, Yang Xu wrote: > Add a dedicated helper to handle the setgid bit when creating a new file > in a setgid directory. This is a preparatory patch for moving setgid > stripping into the vfs. The patch contains no functional changes. > > Currently the setgid stripping logic is open-coded directly in > inode_init_owner() and the individual filesystems are responsible for > handling setgid inheritance. Since this has proven to be brittle as > evidenced by old issues we uncovered over the last months (see [1] to > [3] below) we will try to move this logic into the vfs. > > Link: e014f37db1a2 ("xfs: use setattr_copy to set vfs inode attributes") [1] > Link: 01ea173e103e ("xfs: fix up non-directory creation in SGID directories") [2] > Link: fd84bfdddd16 ("ceph: fix up non-directory creation in SGID directories") [3] > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org> > Reviewed-and-Tested-by: Jeff Layton <jlayton@kernel.org> > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > --- Fyi, I'm on vacation this week and will review this when I get back and if things look sane plan to pick it up for the next mw. Christian
On Thu, Jul 14, 2022 at 02:56:26PM +0200, Christian Brauner wrote: > On Thu, Jul 14, 2022 at 02:11:25PM +0800, Yang Xu wrote: > > Add a dedicated helper to handle the setgid bit when creating a new file > > in a setgid directory. This is a preparatory patch for moving setgid > > stripping into the vfs. The patch contains no functional changes. > > > > Currently the setgid stripping logic is open-coded directly in > > inode_init_owner() and the individual filesystems are responsible for > > handling setgid inheritance. Since this has proven to be brittle as > > evidenced by old issues we uncovered over the last months (see [1] to > > [3] below) we will try to move this logic into the vfs. > > > > Link: e014f37db1a2 ("xfs: use setattr_copy to set vfs inode attributes") [1] > > Link: 01ea173e103e ("xfs: fix up non-directory creation in SGID directories") [2] > > Link: fd84bfdddd16 ("ceph: fix up non-directory creation in SGID directories") [3] > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org> > > Reviewed-and-Tested-by: Jeff Layton <jlayton@kernel.org> > > Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> > > --- > > Fyi, I'm on vacation this week and will review this when I get back and > if things look sane plan to pick it up for the next mw. Getting back I immediately got a massive summer cold so I'm fairly slow at picking things back up. Sorry for the delays. So I rewrote parts of the commit message and specifically added more details to explicitly point out what regression risks exists. But overall I don't see any big issues with this anymore. A full xfstests run for both xfs and ext4 didn't show any regressions. The full LTP testsuite also didn't find anything to complain about. Still doesn't mean we won't have people yell but hey it's a start. I think the benefits of moving S_ISGID handling into the VFS are fairly obvious and described in detail in [3/4]. Weighing benefits vs regression risks it seems that we are inclined to try this approach. We might just fall flat on our face with this but then we'll just have to revert. As always, if someone else wants to get their fingers burned by proposing this during the next mw I'll happily drop it. [1/4] fs: add mode_strip_sgid() helper commit: 2b3416ceff5e6bd4922f6d1c61fb68113dd82302 [2/4] fs: Add missing umask strip in vfs_tmpfile commit: ac6800e279a22b28f4fc21439843025a0d5bf03e [3/4] fs: move S_ISGID stripping into the vfs_*() helpers commit: 1639a49ccdce58ea248841ed9b23babcce6dbb0b [4/4] ceph: rely on vfs for setgid stripping commit: 5fadbd992996e9dda7ebcb62f5352866057bd619 Thanks! Christian
diff --git a/fs/inode.c b/fs/inode.c index bd4da9c5207e..133bf018d6ee 100644 --- a/fs/inode.c +++ b/fs/inode.c @@ -2246,10 +2246,8 @@ void inode_init_owner(struct user_namespace *mnt_userns, struct inode *inode, /* Directories are special, and always inherit S_ISGID */ if (S_ISDIR(mode)) mode |= S_ISGID; - else if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) && - !in_group_p(i_gid_into_mnt(mnt_userns, dir)) && - !capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID)) - mode &= ~S_ISGID; + else + mode = mode_strip_sgid(mnt_userns, dir, mode); } else inode_fsgid_set(inode, mnt_userns); inode->i_mode = mode; @@ -2405,3 +2403,34 @@ struct timespec64 current_time(struct inode *inode) return timestamp_truncate(now, inode); } EXPORT_SYMBOL(current_time); + +/** + * mode_strip_sgid - handle the sgid bit for non-directories + * @mnt_userns: User namespace of the mount the inode was created from + * @dir: parent directory inode + * @mode: mode of the file to be created in @dir + * + * If the @mode of the new file has both the S_ISGID and S_IXGRP bit + * raised and @dir has the S_ISGID bit raised ensure that the caller is + * either in the group of the parent directory or they have CAP_FSETID + * in their user namespace and are privileged over the parent directory. + * In all other cases, strip the S_ISGID bit from @mode. + * + * Return: the new mode to use for the file + */ +umode_t mode_strip_sgid(struct user_namespace *mnt_userns, + const struct inode *dir, umode_t mode) +{ + if ((mode & (S_ISGID | S_IXGRP)) != (S_ISGID | S_IXGRP)) + return mode; + if (S_ISDIR(mode) || !dir || !(dir->i_mode & S_ISGID)) + return mode; + if (in_group_p(i_gid_into_mnt(mnt_userns, dir))) + return mode; + if (capable_wrt_inode_uidgid(mnt_userns, dir, CAP_FSETID)) + return mode; + + mode &= ~S_ISGID; + return mode; +} +EXPORT_SYMBOL(mode_strip_sgid); diff --git a/include/linux/fs.h b/include/linux/fs.h index 9ad5e3520fae..50642668c60f 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1903,6 +1903,8 @@ 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); +umode_t mode_strip_sgid(struct user_namespace *mnt_userns, + const struct inode *dir, umode_t mode); /* * This is the "filldir" function type, used by readdir() to let
Add a dedicated helper to handle the setgid bit when creating a new file in a setgid directory. This is a preparatory patch for moving setgid stripping into the vfs. The patch contains no functional changes. Currently the setgid stripping logic is open-coded directly in inode_init_owner() and the individual filesystems are responsible for handling setgid inheritance. Since this has proven to be brittle as evidenced by old issues we uncovered over the last months (see [1] to [3] below) we will try to move this logic into the vfs. Link: e014f37db1a2 ("xfs: use setattr_copy to set vfs inode attributes") [1] Link: 01ea173e103e ("xfs: fix up non-directory creation in SGID directories") [2] Link: fd84bfdddd16 ("ceph: fix up non-directory creation in SGID directories") [3] Reviewed-by: Darrick J. Wong <djwong@kernel.org> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org> Reviewed-and-Tested-by: Jeff Layton <jlayton@kernel.org> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com> --- fs/inode.c | 37 +++++++++++++++++++++++++++++++++---- include/linux/fs.h | 2 ++ 2 files changed, 35 insertions(+), 4 deletions(-)