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

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

[Amir Goldstein]

From: Christoph Hellwig <hch@lst.de>

commit 01ea173e103edd5ec41acec65b9261b87e123fc2 upstream.

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>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Acked-off-by: Darrick J. Wong <djwong@kernel.org>
---

Hi Greg,

This is an old debt of a patch that was dropped during review of my
batch of 5.10.y xfs backports from v5.12 [1].

Recently, Varsha requested the inclusion of this fix in 5.10.y
and Darrick has Acked it [2].

I have another series of SGID related fixes that also apply to 5.15.y
which I am collaborating on testing with Leah, but as both Christian and
Christoph commented in the original patch review [3], this fix from
v5.12 is independent of the rest of the SGID fixes and is well worth
backporting.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-xfs/20220606143255.685988-1-amir73il@gmail.com/
[2] https://lore.kernel.org/linux-xfs/YyIDzPTn99XLTCFp@magnolia/
[3] https://lore.kernel.org/linux-xfs/20220608082654.GA16753@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 929ed3bc5619..19008838df76 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -802,6 +802,7 @@ xfs_ialloc(
 	xfs_buf_t	**ialloc_context,
 	xfs_inode_t	**ipp)
 {
+	struct inode	*dir = pip ? VFS_I(pip) : NULL;
 	struct xfs_mount *mp = tp->t_mountp;
 	xfs_ino_t	ino;
 	xfs_inode_t	*ip;
@@ -847,18 +848,17 @@ xfs_ialloc(
 		return error;
 	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.25.1

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

[Darrick J. Wong]

On Thu, Sep 22, 2022 at 11:49:56AM +0300, Amir Goldstein wrote:
> From: Christoph Hellwig <hch@lst.de>
> 
> commit 01ea173e103edd5ec41acec65b9261b87e123fc2 upstream.
> 
> 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>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Acked-off-by: Darrick J. Wong <djwong@kernel.org>

(H)acked-off-by?  I suppose we /are/ grafting bits of trees... :D

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

--D

> ---
> 
> Hi Greg,
> 
> This is an old debt of a patch that was dropped during review of my
> batch of 5.10.y xfs backports from v5.12 [1].
> 
> Recently, Varsha requested the inclusion of this fix in 5.10.y
> and Darrick has Acked it [2].
> 
> I have another series of SGID related fixes that also apply to 5.15.y
> which I am collaborating on testing with Leah, but as both Christian and
> Christoph commented in the original patch review [3], this fix from
> v5.12 is independent of the rest of the SGID fixes and is well worth
> backporting.
> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-xfs/20220606143255.685988-1-amir73il@gmail.com/
> [2] https://lore.kernel.org/linux-xfs/YyIDzPTn99XLTCFp@magnolia/
> [3] https://lore.kernel.org/linux-xfs/20220608082654.GA16753@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 929ed3bc5619..19008838df76 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -802,6 +802,7 @@ xfs_ialloc(
>  	xfs_buf_t	**ialloc_context,
>  	xfs_inode_t	**ipp)
>  {
> +	struct inode	*dir = pip ? VFS_I(pip) : NULL;
>  	struct xfs_mount *mp = tp->t_mountp;
>  	xfs_ino_t	ino;
>  	xfs_inode_t	*ip;
> @@ -847,18 +848,17 @@ xfs_ialloc(
>  		return error;
>  	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.25.1
> 

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

[Greg Kroah-Hartman]

On Thu, Sep 22, 2022 at 08:21:29AM -0700, Darrick J. Wong wrote:
> On Thu, Sep 22, 2022 at 11:49:56AM +0300, Amir Goldstein wrote:
> > From: Christoph Hellwig <hch@lst.de>
> > 
> > commit 01ea173e103edd5ec41acec65b9261b87e123fc2 upstream.
> > 
> > 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>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > Acked-off-by: Darrick J. Wong <djwong@kernel.org>
> 
> (H)acked-off-by?  I suppose we /are/ grafting bits of trees... :D
> 
> Acked-by: Darrick J. Wong <djwong@kernel.org>

Now queued up, thanks.

greg k-h