diff mbox series

[v7,1/4] fs: move sgid stripping operation from inode_init_owner into mode_strip_sgid

Message ID 1650946792-9545-1-git-send-email-xuyang2018.jy@fujitsu.com (mailing list archive)
State New, archived
Headers show
Series [v7,1/4] fs: move sgid stripping operation from inode_init_owner into mode_strip_sgid | expand

Commit Message

Yang Xu (Fujitsu) April 26, 2022, 4:19 a.m. UTC 
This has no functional change. Just create and export mode_strip_sgid
api for the subsequent patch. This function is used to strip S_ISGID mode
when init a new inode.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christian Brauner (Microsoft) <brauner@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(-)

Comments

Christian Brauner April 26, 2022, 7:06 a.m. UTC | #1 
On Tue, Apr 26, 2022 at 12:19:49PM +0800, Yang Xu wrote:
> This has no functional change. Just create and export mode_strip_sgid
> api for the subsequent patch. This function is used to strip S_ISGID mode
> when init a new inode.
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Yang Xu <xuyang2018.jy@fujitsu.com>
> ---

Since this is a very sensitive patch series I think we need to be
annoyingly pedantic about the commit messages. This is really only
necessary because of the nature of these changes so you'll forgive me
for being really annoying about this. Here's what I'd change the commit
message to:

fs: add mode_strip_sgid() helper

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]
Yang Xu (Fujitsu) April 26, 2022, 7:39 a.m. UTC | #2 
on 2022/4/26 15:06, Christian Brauner wrote:
> On Tue, Apr 26, 2022 at 12:19:49PM +0800, Yang Xu wrote:
>> This has no functional change. Just create and export mode_strip_sgid
>> api for the subsequent patch. This function is used to strip S_ISGID mode
>> when init a new inode.
>>
>> Reviewed-by: Darrick J. Wong<djwong@kernel.org>
>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>> ---
>
> Since this is a very sensitive patch series I think we need to be
> annoyingly pedantic about the commit messages. This is really only
> necessary because of the nature of these changes so you'll forgive me
> for being really annoying about this. Here's what I'd change the commit
> message to:
>
> fs: add mode_strip_sgid() helper
>
> 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]

This seems better, thanks.

ps: Sorry, forgive my poor ability for write this.
Christian Brauner April 26, 2022, 8:39 a.m. UTC | #3 
On Tue, Apr 26, 2022 at 07:39:07AM +0000, xuyang2018.jy@fujitsu.com wrote:
> on 2022/4/26 15:06, Christian Brauner wrote:
> > On Tue, Apr 26, 2022 at 12:19:49PM +0800, Yang Xu wrote:
> >> This has no functional change. Just create and export mode_strip_sgid
> >> api for the subsequent patch. This function is used to strip S_ISGID mode
> >> when init a new inode.
> >>
> >> Reviewed-by: Darrick J. Wong<djwong@kernel.org>
> >> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
> >> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
> >> ---
> >
> > Since this is a very sensitive patch series I think we need to be
> > annoyingly pedantic about the commit messages. This is really only
> > necessary because of the nature of these changes so you'll forgive me
> > for being really annoying about this. Here's what I'd change the commit
> > message to:
> >
> > fs: add mode_strip_sgid() helper
> >
> > 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]
> 
> This seems better, thanks.
> 
> ps: Sorry, forgive my poor ability for write this.

This really isn't any comment on your ability to write this! I tried to
make this clear but I obviously failed.

It is really just that this has an associated non-zero regression risk
and we need to make sure to highlight this and be very clear about the
motivation for this change. So it's equal parts pedantry and trying to
keep our own heads off the guillotine.
Yang Xu (Fujitsu) April 26, 2022, 9:22 a.m. UTC | #4 
on 2022/4/26 16:39, Christian Brauner wrote:
> On Tue, Apr 26, 2022 at 07:39:07AM +0000, xuyang2018.jy@fujitsu.com wrote:
>> on 2022/4/26 15:06, Christian Brauner wrote:
>>> On Tue, Apr 26, 2022 at 12:19:49PM +0800, Yang Xu wrote:
>>>> This has no functional change. Just create and export mode_strip_sgid
>>>> api for the subsequent patch. This function is used to strip S_ISGID mode
>>>> when init a new inode.
>>>>
>>>> Reviewed-by: Darrick J. Wong<djwong@kernel.org>
>>>> Reviewed-by: Christian Brauner (Microsoft)<brauner@kernel.org>
>>>> Signed-off-by: Yang Xu<xuyang2018.jy@fujitsu.com>
>>>> ---
>>>
>>> Since this is a very sensitive patch series I think we need to be
>>> annoyingly pedantic about the commit messages. This is really only
>>> necessary because of the nature of these changes so you'll forgive me
>>> for being really annoying about this. Here's what I'd change the commit
>>> message to:
>>>
>>> fs: add mode_strip_sgid() helper
>>>
>>> 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]
>>
>> This seems better, thanks.
>>
>> ps: Sorry, forgive my poor ability for write this.
>
> This really isn't any comment on your ability to write this! I tried to
> make this clear but I obviously failed.
>
> It is really just that this has an associated non-zero regression risk
> and we need to make sure to highlight this and be very clear about the
> motivation for this change. So it's equal parts pedantry and trying to
> keep our own heads off the guillotine.

Understand. So do you have other comments? I plan to send a v8(based on 
5.18-rc4).
diff mbox series

Patch

diff --git a/fs/inode.c b/fs/inode.c
index 9d9b422504d1..e9a5f2ec2f89 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 (S_ISDIR(mode) || !dir || !(dir->i_mode & S_ISGID))
+		return mode;
+	if ((mode & (S_ISGID | S_IXGRP)) != (S_ISGID | S_IXGRP))
+		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 bbde95387a23..98b44a2732f5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1897,6 +1897,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