xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 7/9] xfs: kill suid/sgid through the truncate path.
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Thu, 30 May 2013 10:17:30 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1369636707-15150-8-git-send-email-david@xxxxxxxxxxxxx>
References: <1369636707-15150-1-git-send-email-david@xxxxxxxxxxxxx> <1369636707-15150-8-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6
On 05/27/2013 02:38 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>

> Fix it.
> 
> cc: stable kernel <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
>  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;
> 

<Prev in Thread] Current Thread [Next in Thread>