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

["Darrick J. Wong" <djwong@kernel.org>]

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
>