Request to cherry-pick 01ea173e103edd5ec41acec65b9261b87e123fc2 to v5.10

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: 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