On Thu, Sep 22, 2022 at 11:49:56AM +0300, Amir Goldstein wrote:
> From: Christoph Hellwig <hch@xxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> Acked-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
(H)acked-off-by? I suppose we /are/ grafting bits of trees... :D
Acked-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--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@xxxxxxxxx/
> [2] https://lore.kernel.org/linux-xfs/YyIDzPTn99XLTCFp@magnolia/
> [3] https://lore.kernel.org/linux-xfs/20220608082654.GA16753@xxxxxx/
>
> 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
>
