| Message ID | 20220922084956.74262-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [5.10] xfs: fix up non-directory creation in SGID directories | expand |
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 >
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
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); } /*