[PATCH] xfs: fix up non-directory creation in SGID directories

[PATCH] xfs: fix up non-directory creation in SGID directories

[Christoph Hellwig]

XFS always inherits the SGID bit if it is set on the parent inode, while
the generic inode_init_owner does not do this in a few cases where it can
create a possible security problem, see commit 0fa3ecd87848
("Fix up non-directory creation in SGID directories") for details.

Switch XFS to use the generic helper for the normal path to fix this,
just keeping the simple field inheritance open coded for the case of the
non-sgid case with the bsdgrpid mount option.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_inode.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index b7352bc4c81527..7289325aa5c7c0 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -775,6 +775,7 @@ xfs_init_new_inode(
 	prid_t			prid,
 	struct xfs_inode	**ipp)
 {
+	struct inode		*dir = pip ? VFS_I(pip) : NULL;
 	struct xfs_mount	*mp = tp->t_mountp;
 	struct xfs_inode	*ip;
 	unsigned int		flags;
@@ -804,18 +805,17 @@ xfs_init_new_inode(
 
 	ASSERT(ip != NULL);
 	inode = VFS_I(ip);
-	inode->i_mode = mode;
 	set_nlink(inode, nlink);
-	inode->i_uid = current_fsuid();
 	inode->i_rdev = rdev;
 	ip->i_d.di_projid = prid;
 
-	if (pip && XFS_INHERIT_GID(pip)) {
-		inode->i_gid = VFS_I(pip)->i_gid;
-		if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
-			inode->i_mode |= S_ISGID;
+	if (dir && !(dir->i_mode & S_ISGID) &&
+	    (mp->m_flags & XFS_MOUNT_GRPID)) {
+		inode->i_uid = current_fsuid();
+		inode->i_gid = dir->i_gid;
+		inode->i_mode = mode;
 	} else {
-		inode->i_gid = current_fsgid();
+		inode_init_owner(inode, dir, mode);
 	}
 
 	/*
-- 
2.29.2

Re: [PATCH] xfs: fix up non-directory creation in SGID directories

[Christian Brauner]

On Wed, Jan 13, 2021 at 07:46:30PM +0100, Christoph Hellwig wrote:
> XFS always inherits the SGID bit if it is set on the parent inode, while
> the generic inode_init_owner does not do this in a few cases where it can
> create a possible security problem, see commit 0fa3ecd87848
> ("Fix up non-directory creation in SGID directories") for details.
> 
> Switch XFS to use the generic helper for the normal path to fix this,
> just keeping the simple field inheritance open coded for the case of the
> non-sgid case with the bsdgrpid mount option.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>

I ran the idmapped mounts xfstests on this patchset. With this patch
applied I was able to remove the special casing for xfs (apart from the
irix compatibility check) and got clean test runs:


1. with regular setgid inheritance rules
root@f2-vm:/xfstests# ./check generic/622
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 f2-vm 5.11.0-rc3-brauner-idmapped-mounts-xfs #311 SMP Thu Jan 14 09:55:14 UTC 2021
MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop7
MOUNT_OPTIONS -- /dev/loop7 /mnt/scratch

generic/622 1s ...  2s
Ran: generic/622
Passed all 1 tests

2. with irix_sgid_inherit setgid inheritance rules
root@f2-vm:/xfstests# echo 1 > /proc/sys/fs/xfs/irix_sgid_inherit
root@f2-vm:/xfstests# ./check generic/622
FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 f2-vm 5.11.0-rc3-brauner-idmapped-mounts-xfs #311 SMP Thu Jan 14 09:55:14 UTC 2021
MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop7
MOUNT_OPTIONS -- /dev/loop7 /mnt/scratch

generic/622 2s ...  1s
Ran: generic/622
Passed all 1 tests

Thanks!
Christian

Re: [PATCH] xfs: fix up non-directory creation in SGID directories

[Darrick J. Wong]

On Thu, Jan 14, 2021 at 11:45:11AM +0100, Christian Brauner wrote:
> On Wed, Jan 13, 2021 at 07:46:30PM +0100, Christoph Hellwig wrote:
> > XFS always inherits the SGID bit if it is set on the parent inode, while
> > the generic inode_init_owner does not do this in a few cases where it can
> > create a possible security problem, see commit 0fa3ecd87848
> > ("Fix up non-directory creation in SGID directories") for details.
> > 
> > Switch XFS to use the generic helper for the normal path to fix this,
> > just keeping the simple field inheritance open coded for the case of the
> > non-sgid case with the bsdgrpid mount option.
> > 
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> 
> Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> 
> I ran the idmapped mounts xfstests on this patchset. With this patch
> applied I was able to remove the special casing for xfs (apart from the
> irix compatibility check) and got clean test runs:
> 
> 
> 1. with regular setgid inheritance rules
> root@f2-vm:/xfstests# ./check generic/622

Is this test posted to fstests somewhere?

FWIW the code change looks reasonable to me, but I wanted to see the
functionality exercise test first. :)

--D

> FSTYP         -- xfs (non-debug)
> PLATFORM      -- Linux/x86_64 f2-vm 5.11.0-rc3-brauner-idmapped-mounts-xfs #311 SMP Thu Jan 14 09:55:14 UTC 2021
> MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop7
> MOUNT_OPTIONS -- /dev/loop7 /mnt/scratch
> 
> generic/622 1s ...  2s
> Ran: generic/622
> Passed all 1 tests
> 
> 2. with irix_sgid_inherit setgid inheritance rules
> root@f2-vm:/xfstests# echo 1 > /proc/sys/fs/xfs/irix_sgid_inherit
> root@f2-vm:/xfstests# ./check generic/622
> FSTYP         -- xfs (non-debug)
> PLATFORM      -- Linux/x86_64 f2-vm 5.11.0-rc3-brauner-idmapped-mounts-xfs #311 SMP Thu Jan 14 09:55:14 UTC 2021
> MKFS_OPTIONS  -- -f -bsize=4096 /dev/loop7
> MOUNT_OPTIONS -- /dev/loop7 /mnt/scratch
> 
> generic/622 2s ...  1s
> Ran: generic/622
> Passed all 1 tests
> 
> Thanks!
> Christian

Re: [PATCH] xfs: fix up non-directory creation in SGID directories

[Christian Brauner]

On Thu, Jan 14, 2021 at 08:06:44AM -0800, Darrick J. Wong wrote:
> On Thu, Jan 14, 2021 at 11:45:11AM +0100, Christian Brauner wrote:
> > On Wed, Jan 13, 2021 at 07:46:30PM +0100, Christoph Hellwig wrote:
> > > XFS always inherits the SGID bit if it is set on the parent inode, while
> > > the generic inode_init_owner does not do this in a few cases where it can
> > > create a possible security problem, see commit 0fa3ecd87848
> > > ("Fix up non-directory creation in SGID directories") for details.
> > > 
> > > Switch XFS to use the generic helper for the normal path to fix this,
> > > just keeping the simple field inheritance open coded for the case of the
> > > non-sgid case with the bsdgrpid mount option.
> > > 
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > 
> > Reviewed-by: Christian Brauner <christian.brauner@ubuntu.com>
> > 
> > I ran the idmapped mounts xfstests on this patchset. With this patch
> > applied I was able to remove the special casing for xfs (apart from the
> > irix compatibility check) and got clean test runs:
> > 
> > 
> > 1. with regular setgid inheritance rules
> > root@f2-vm:/xfstests# ./check generic/622
> 
> Is this test posted to fstests somewhere?
> 
> FWIW the code change looks reasonable to me, but I wanted to see the
> functionality exercise test first. :)

(The test is part of the idmapped mount testsuite. This will be sent for
 inclusion after the v5.12 merge window has closed.)

I'll paste the test here (The one on list still works around the xfs
bug and is thus not suitable for testing a fixed xfs. :)) and the
complete test is found under [1]:

/* The following tests are concerned with setgid inheritance. These can be
 * filesystem type specific. For xfs, if a new file or directory is created
 * within a setgid directory and irix_sgid_inhiert is set then inherit the
 * setgid bit if the caller is in the group of the directory.
 */
static int setgid_create(void)
{
	int fret = -1;
	int file1_fd = -EBADF;
	pid_t pid;

	if (!caps_supported())
		return 0;

	if (fchmod(t_dir1_fd, S_IRUSR |
			      S_IWUSR |
			      S_IRGRP |
			      S_IWGRP |
			      S_IROTH |
			      S_IWOTH |
			      S_IXUSR |
			      S_IXGRP |
			      S_IXOTH |
			      S_ISGID), 0) {
		log_stderr("failure: fchmod");
		goto out;
	}

	/* Verify that the setgid bit got raised. */
	if (!is_setgid(t_dir1_fd, "", AT_EMPTY_PATH)) {
		log_stderr("failure: is_setgid");
		goto out;
	}

	pid = fork();
	if (pid < 0) {
		log_stderr("failure: fork");
		goto out;
	}
	if (pid == 0) {
		/* create regular file via open() */
		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
		if (file1_fd < 0)
			die("failure: create");

		/* We're capable_wrt_inode_uidgid() and also our fsgid matches
		 * the directories gid.
		 */
		if (!is_setgid(t_dir1_fd, FILE1, 0))
			die("failure: is_setgid");

		/* create directory */
		if (mkdirat(t_dir1_fd, DIR1, 0000))
			die("failure: create");

		/* Directories always inherit the setgid bit. */
		if (!is_setgid(t_dir1_fd, DIR1, 0))
			die("failure: is_setgid");

		if (unlinkat(t_dir1_fd, FILE1, 0))
			die("failure: delete");

		if (unlinkat(t_dir1_fd, DIR1, AT_REMOVEDIR))
			die("failure: delete");

		exit(EXIT_SUCCESS);
	}
	if (wait_for_pid(pid))
		goto out;

	pid = fork();
	if (pid < 0) {
		log_stderr("failure: fork");
		goto out;
	}
	if (pid == 0) {
		if (!switch_ids(0, 10000))
			die("failure: switch_ids");

		if (!caps_down())
			die("failure: caps_down");

		/* create regular file via open() */
		file1_fd = openat(t_dir1_fd, FILE1, O_CREAT | O_EXCL | O_CLOEXEC, S_IXGRP | S_ISGID);
		if (file1_fd < 0)
			die("failure: create");

		/* Neither in_group_p() nor capable_wrt_inode_uidgid() so setgid
		 * bit needs to be stripped.
		 */
		if (is_setgid(t_dir1_fd, FILE1, 0))
			die("failure: is_setgid");

		/* create directory */
		if (mkdirat(t_dir1_fd, DIR1, 0000))
			die("failure: create");

		if (xfs_irix_sgid_inherit_enabled()) {
			/* We're not in_group_p(). */
			if (is_setgid(t_dir1_fd, DIR1, 0))
				die("failure: is_setgid");
		} else {
			/* Directories always inherit the setgid bit. */
			if (!is_setgid(t_dir1_fd, DIR1, 0))
				die("failure: is_setgid");
		}

		exit(EXIT_SUCCESS);
	}
	if (wait_for_pid(pid))
		goto out;

	fret = 0;
out:
	safe_close(file1_fd);

	return fret;
}

[1]: https://gitlab.com/brauner/xfstests/-/commit/d1859aecc341edeefe712ab8a3fa63159356c119.patch

and specifically

Request to cherry-pick 01ea173e103edd5ec41acec65b9261b87e123fc2 to v5.10

[Varsha Teratipally]

Hi all,

Commit 01ea173e103edd5ec41acec65b9261b87e123fc2 (upstream: xfs: fix up
non-directory creation in SGID directories) fixes an issue where in xfs
sometimes, a local user could create files with an unitended group
permissions as an owner and execution where a directory is SGID and belongs to a certain group and is writable by a user who is not a member of this group and seems like a good candidate for the v5.10 stable tree given that 5.10 is used in versions of debian, ubuntu.

This patch applies cleanly. Let me know what you think

[PATCH] xfs: fix up non-directory creation in SGID directories

[Varsha Teratipally]

From: Christoph Hellwig <hch@lst.de>

XFS always inherits the SGID bit if it is set on the parent inode, while
the generic inode_init_owner does not do this in a few cases where it can
create a possible security problem, see commit 0fa3ecd87848
("Fix up non-directory creation in SGID directories") for details.

Switch XFS to use the generic helper for the normal path to fix this,
just keeping the simple field inheritance open coded for the case of the
non-sgid case with the bsdgrpid mount option.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_inode.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 8ebd9c64aa48..e2a1db4cee43 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -775,6 +775,7 @@ xfs_init_new_inode(
 	prid_t			prid,
 	struct xfs_inode	**ipp)
 {
+	struct inode		*dir = pip ? VFS_I(pip) : NULL;
 	struct xfs_mount	*mp = tp->t_mountp;
 	struct xfs_inode	*ip;
 	unsigned int		flags;
@@ -804,18 +805,17 @@ xfs_init_new_inode(
 
 	ASSERT(ip != NULL);
 	inode = VFS_I(ip);
-	inode->i_mode = mode;
 	set_nlink(inode, nlink);
-	inode->i_uid = current_fsuid();
 	inode->i_rdev = rdev;
 	ip->i_d.di_projid = prid;
 
-	if (pip && XFS_INHERIT_GID(pip)) {
-		inode->i_gid = VFS_I(pip)->i_gid;
-		if ((VFS_I(pip)->i_mode & S_ISGID) && S_ISDIR(mode))
-			inode->i_mode |= S_ISGID;
+	if (dir && !(dir->i_mode & S_ISGID) &&
+	    (mp->m_flags & XFS_MOUNT_GRPID)) {
+		inode->i_uid = current_fsuid();
+		inode->i_gid = dir->i_gid;
+		inode->i_mode = mode;
 	} else {
-		inode->i_gid = current_fsgid();
+		inode_init_owner(inode, dir, mode);
 	}
 
 	/*
-- 
2.37.2.789.g6183377224-goog

Re: [PATCH] xfs: fix up non-directory creation in SGID directories

[Amir Goldstein]

On Tue, Sep 6, 2022 at 9:36 PM Varsha Teratipally
<teratipally@google.com> wrote:
>
> From: Christoph Hellwig <hch@lst.de>
>
> XFS always inherits the SGID bit if it is set on the parent inode, while
> the generic inode_init_owner does not do this in a few cases where it can
> create a possible security problem, see commit 0fa3ecd87848
> ("Fix up non-directory creation in SGID directories") for details.
>
> Switch XFS to use the generic helper for the normal path to fix this,
> just keeping the simple field inheritance open coded for the case of the
> non-sgid case with the bsdgrpid mount option.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Hi Varsha,

For future reference, when posting an xfs patch for stable,
please follow these guidelines:

1. Post it to xfs list for review BEFORE posting to stable
2. LKML is not a relevant list
3. Tag the patch with the target kernel [PATCH 5.10]
4. Include the upstream commit id
5. Add some description (after --- line) about how you tested

Regarding this specific patch for 5.10, I had already tested and posted it
for review back in June [1].

Dave Chinner commented then that he was concerned about other
security issues discovered later on related to the generic implementation
of SGID stripping.
At the time, the generic upstream fixes and tests were still WIP.

Christoph Hellwig, the author of the original patch replied to Dave's
concern:

"To me backporting it seems good and useful, as it fixes a relatively
big problem.  The remaining issues seem minor compared to that."

Christiain Brauner who has been reviewing the generic upstream
also agreed that:

"Imho, backporting this patch is useful. It fixes a basic issue."

So this specific fix patch from the v5.12 release, which is not
relevant for 5.15.y has my blessing to go to 5.10.y.

Regardless, the last bits of the upstream work on the generic
implementation by Yang Xu have landed in v6.0-rc1 [2] and the
respective fstests have just recently landed in fstests v2022.09.04.

I already have all the patches backported to 5.10 [3] and will start
testing them in the following weeks, but now I also depend on Leah
to test them for 5.15.y before I can post to 5.10.y and that may take
a while...

Thanks,
Amir.

[1] https://lore.kernel.org/linux-xfs/CAOQ4uxg4=m9zEFbDAKXx7CP7HYiMwtsYSJvq076oKpy-OhK1uw@mail.gmail.com/
[2] https://lore.kernel.org/linux-fsdevel/20220809103957.1851931-1-brauner@kernel.org/
[3] https://github.com/amir73il/linux/commits/xfs-5.10.y-sgid-fixes

Re: [PATCH] xfs: fix up non-directory creation in SGID directories

[Amir Goldstein]

[Fix CC for brauner]

On Wed, Sep 7, 2022 at 10:40 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Sep 6, 2022 at 9:36 PM Varsha Teratipally
> <teratipally@google.com> wrote:
> >
> > From: Christoph Hellwig <hch@lst.de>
> >
> > XFS always inherits the SGID bit if it is set on the parent inode, while
> > the generic inode_init_owner does not do this in a few cases where it can
> > create a possible security problem, see commit 0fa3ecd87848
> > ("Fix up non-directory creation in SGID directories") for details.
> >
> > Switch XFS to use the generic helper for the normal path to fix this,
> > just keeping the simple field inheritance open coded for the case of the
> > non-sgid case with the bsdgrpid mount option.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
>
> Hi Varsha,
>
> For future reference, when posting an xfs patch for stable,
> please follow these guidelines:
>
> 1. Post it to xfs list for review BEFORE posting to stable
> 2. LKML is not a relevant list
> 3. Tag the patch with the target kernel [PATCH 5.10]
> 4. Include the upstream commit id
> 5. Add some description (after --- line) about how you tested
>
> Regarding this specific patch for 5.10, I had already tested and posted it
> for review back in June [1].
>
> Dave Chinner commented then that he was concerned about other
> security issues discovered later on related to the generic implementation
> of SGID stripping.
> At the time, the generic upstream fixes and tests were still WIP.
>
> Christoph Hellwig, the author of the original patch replied to Dave's
> concern:
>
> "To me backporting it seems good and useful, as it fixes a relatively
> big problem.  The remaining issues seem minor compared to that."
>
> Christiain Brauner who has been reviewing the generic upstream
> also agreed that:
>
> "Imho, backporting this patch is useful. It fixes a basic issue."
>
> So this specific fix patch from the v5.12 release, which is not
> relevant for 5.15.y has my blessing to go to 5.10.y.
>
> Regardless, the last bits of the upstream work on the generic
> implementation by Yang Xu have landed in v6.0-rc1 [2] and the
> respective fstests have just recently landed in fstests v2022.09.04.
>
> I already have all the patches backported to 5.10 [3] and will start
> testing them in the following weeks, but now I also depend on Leah
> to test them for 5.15.y before I can post to 5.10.y and that may take
> a while...
>
> Thanks,
> Amir.
>
> [1] https://lore.kernel.org/linux-xfs/CAOQ4uxg4=m9zEFbDAKXx7CP7HYiMwtsYSJvq076oKpy-OhK1uw@mail.gmail.com/
> [2] https://lore.kernel.org/linux-fsdevel/20220809103957.1851931-1-brauner@kernel.org/
> [3] https://github.com/amir73il/linux/commits/xfs-5.10.y-sgid-fixes

Re: Request to cherry-pick 01ea173e103edd5ec41acec65b9261b87e123fc2 to v5.10

[Amir Goldstein]

On Tue, Sep 6, 2022 at 9:36 PM Varsha Teratipally
<teratipally@google.com> wrote:
>
> Hi all,
>
> Commit 01ea173e103edd5ec41acec65b9261b87e123fc2 (upstream: xfs: fix up
> non-directory creation in SGID directories) fixes an issue where in xfs
> sometimes, a local user could create files with an unitended group
> permissions as an owner and execution where a directory is SGID and belongs to a certain group and is writable by a user who is not a member of this group and seems like a good candidate for the v5.10 stable tree given that 5.10 is used in versions of debian, ubuntu.
>
> This patch applies cleanly. Let me know what you think
>

Since you already posted the patch, I wrote what I think on the post:

https://lore.kernel.org/linux-xfs/CAOQ4uxi_Q8aXUg+FM0Q9__t=KqJSVqOgkS8j8kNC3MQfniZLWA@mail.gmail.com/

Bottom line - I think that the patch should be applied to 5.10.y
without further delay.

Thanks,
Amir.

Re: [PATCH] xfs: fix up non-directory creation in SGID directories

[Greg KH]

On Tue, Sep 06, 2022 at 06:36:00PM +0000, Varsha Teratipally wrote:
> From: Christoph Hellwig <hch@lst.de>
> 
> XFS always inherits the SGID bit if it is set on the parent inode, while
> the generic inode_init_owner does not do this in a few cases where it can
> create a possible security problem, see commit 0fa3ecd87848
> ("Fix up non-directory creation in SGID directories") for details.
> 
> Switch XFS to use the generic helper for the normal path to fix this,
> just keeping the simple field inheritance open coded for the case of the
> non-sgid case with the bsdgrpid mount option.
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Why did you not sign off on this if you are forwarding it on?

Also, what is the git id of this commit in Linus's tree (we need that
hint...)

Please fix both up and resend and get the ack of the stable xfs
developers on it as well.

thanks,

greg k-h

Re: [PATCH] xfs: fix up non-directory creation in SGID directories

[Amir Goldstein]

On Thu, Sep 8, 2022 at 2:48 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Sep 06, 2022 at 06:36:00PM +0000, Varsha Teratipally wrote:
> > From: Christoph Hellwig <hch@lst.de>
> >
> > XFS always inherits the SGID bit if it is set on the parent inode, while
> > the generic inode_init_owner does not do this in a few cases where it can
> > create a possible security problem, see commit 0fa3ecd87848
> > ("Fix up non-directory creation in SGID directories") for details.
> >
> > Switch XFS to use the generic helper for the normal path to fix this,
> > just keeping the simple field inheritance open coded for the case of the
> > non-sgid case with the bsdgrpid mount option.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>
> Why did you not sign off on this if you are forwarding it on?
>
> Also, what is the git id of this commit in Linus's tree (we need that
> hint...)
>
> Please fix both up and resend and get the ack of the stable xfs
> developers on it as well.
>

Varsha,

FWIW, I re-tested the patch on top of v5.10.141,
so when re-posting [PATCH 5.10] you may add:

Tested-by: Amir Goldstein <amir73il@gmail.com>

Darrick or Christoph,

Can you please ACK this patch?

Thanks,
Amir.

Re: [PATCH] xfs: fix up non-directory creation in SGID directories

[Darrick J. Wong]

On Thu, Sep 08, 2022 at 03:02:41PM +0300, Amir Goldstein wrote:
> On Thu, Sep 8, 2022 at 2:48 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Tue, Sep 06, 2022 at 06:36:00PM +0000, Varsha Teratipally wrote:
> > > From: Christoph Hellwig <hch@lst.de>
> > >
> > > XFS always inherits the SGID bit if it is set on the parent inode, while
> > > the generic inode_init_owner does not do this in a few cases where it can
> > > create a possible security problem, see commit 0fa3ecd87848
> > > ("Fix up non-directory creation in SGID directories") for details.
> > >
> > > Switch XFS to use the generic helper for the normal path to fix this,
> > > just keeping the simple field inheritance open coded for the case of the
> > > non-sgid case with the bsdgrpid mount option.
> > >
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Reported-by: Christian Brauner <christian.brauner@ubuntu.com>
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> >
> > Why did you not sign off on this if you are forwarding it on?
> >
> > Also, what is the git id of this commit in Linus's tree (we need that
> > hint...)
> >
> > Please fix both up and resend and get the ack of the stable xfs
> > developers on it as well.
> >
> 
> Varsha,
> 
> FWIW, I re-tested the patch on top of v5.10.141,
> so when re-posting [PATCH 5.10] you may add:
> 
> Tested-by: Amir Goldstein <amir73il@gmail.com>
> 
> Darrick or Christoph,
> 
> Can you please ACK this patch?

With all the bookkeepping bits corrected (and assuming that the VFS
fixes have been or are about to be applied):

Acked-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> Thanks,
> Amir.