Re: [PATCH 7/9] xfs: kill suid/sgid through the truncate path.

Brian Foster
2013-05-30T14:20:31Z

On 05/27/2013 02:38 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> XFS has failed to kill suid/sgid bits correctly when truncating
> files of non-zero size since commit c4ed4243 ("xfs: split
> xfs_setattr") introduced in the 3.1 kernel. Fix it.
>

The code makes sense and I can easily hit an assert when truncating
(extending) a suid file on a debug kernel without this patch (and I see
the suid dropped with the patch).

The changelog seems slightly misleading (implying any truncate of a zero
sized file should work, when really only 0->0 size truncates work), but
not a big deal...

Reviewed-by: Brian Foster <bfoster@redhat.com>

> Fix it.
>
> cc: stable kernel <stable@vger.kernel.org>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/xfs_iops.c | 47 ++++++++++++++++++++++++++++++++---------------
> 1 file changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index d82efaa..ca9ecaa 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -455,6 +455,28 @@ xfs_vn_getattr(
> return 0;
> }
>
> +static void
> +xfs_setattr_mode(
> + struct xfs_trans *tp,
> + struct xfs_inode *ip,
> + struct iattr *iattr)
> +{
> + struct inode *inode = VFS_I(ip);
> + umode_t mode = iattr->ia_mode;
> +
> + ASSERT(tp);
> + ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));
> +
> + if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> + mode &= ~S_ISGID;
> +
> + ip->i_d.di_mode &= S_IFMT;
> + ip->i_d.di_mode |= mode & ~S_IFMT;
> +
> + inode->i_mode &= S_IFMT;
> + inode->i_mode |= mode & ~S_IFMT;
> +}
> +
> int
> xfs_setattr_nonsize(
> struct xfs_inode *ip,
> @@ -606,18 +628,8 @@ xfs_setattr_nonsize(
> /*
> * Change file access modes.
> */
> - if (mask & ATTR_MODE) {
> - umode_t mode = iattr->ia_mode;
> -
> - if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> - mode &= ~S_ISGID;
> -
> - ip->i_d.di_mode &= S_IFMT;
> - ip->i_d.di_mode |= mode & ~S_IFMT;
> -
> - inode->i_mode &= S_IFMT;
> - inode->i_mode |= mode & ~S_IFMT;
> - }
> + if (mask & ATTR_MODE)
> + xfs_setattr_mode(tp, ip, iattr);
>
> /*
> * Change file access or modified times.
> @@ -714,9 +726,8 @@ xfs_setattr_size(
> return XFS_ERROR(error);
>
> ASSERT(S_ISREG(ip->i_d.di_mode));
> - ASSERT((mask & (ATTR_MODE|ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> - ATTR_MTIME_SET|ATTR_KILL_SUID|ATTR_KILL_SGID|
> - ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
> + ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> + ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
>
> if (!(flags & XFS_ATTR_NOLOCK)) {
> lock_flags |= XFS_IOLOCK_EXCL;
> @@ -860,6 +871,12 @@ xfs_setattr_size(
> xfs_inode_clear_eofblocks_tag(ip);
> }
>
> + /*
> + * Change file access modes.
> + */
> + if (mask & ATTR_MODE)
> + xfs_setattr_mode(tp, ip, iattr);
> +
> if (mask & ATTR_CTIME) {
> inode->i_ctime = iattr->ia_ctime;
> ip->i_d.di_ctime.t_sec = iattr->ia_ctime.tv_sec;