On Tue, Apr 9, 2019 at 6:19 AM Darrick J. Wong <darrick.wong@xxxxxxxxxx> wrote:
>
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> The chattr manpage has this to say about immutable files:
>
> "A file with the 'i' attribute cannot be modified: it cannot be deleted
> or renamed, no link can be created to this file, most of the file's
> metadata can not be modified, and the file can not be opened in write
> mode."
>
> However, we don't actually check the immutable flag in the setattr code,
> which means that we can update project ids and extent size hints on
> supposedly immutable files. Therefore, reject a setattr call on an
> immutable file except for the case where we're trying to unset
> IMMUTABLE.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/xfs_ioctl.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 5a1b96dad901..67d12027f563 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1023,6 +1023,40 @@ xfs_ioctl_setattr_flush(
> return filemap_write_and_wait(inode->i_mapping);
> }
>
> +/*
> + * If immutable is set and we are not clearing it, we're not allowed to change
> + * anything else in the inode.
This looks correct, but FYI, neither xfs_io nor chattr clears 'immutable'
and sets projid/*extsize in one ioctl/xfsctl, so there is no justification to
making an extra effort to support that use case. You could do with
checking 'immutable' inside xfs_ioctl_setattr_check_projid/*extsize()
and leave only the di_flags check here.
Some would say that will be cleaner code.
Its a matter of taste and its your subsystem, so feel free to dismiss
this comments.
Thanks,
Amir.
> Don't error out if we're only trying to set
> + * immutable on an immutable file.
> + */
> +static int
> +xfs_ioctl_setattr_immutable(
> + struct xfs_inode *ip,
> + struct fsxattr *fa,
> + uint16_t di_flags,
> + uint64_t di_flags2)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> +
> + if (!(ip->i_d.di_flags & XFS_DIFLAG_IMMUTABLE) ||
> + !(fa->fsx_xflags & FS_XFLAG_IMMUTABLE))
> + return 0;
> +
> + if ((ip->i_d.di_flags & ~XFS_DIFLAG_IMMUTABLE) !=
> + (di_flags & ~XFS_DIFLAG_IMMUTABLE))
> + return -EPERM;
> + if (ip->i_d.di_version >= 3 && ip->i_d.di_flags2 != di_flags2)
> + return -EPERM;
> + if (xfs_get_projid(ip) != fa->fsx_projid)
> + return -EPERM;
> + if (ip->i_d.di_extsize != fa->fsx_extsize >> mp->m_sb.sb_blocklog)
> + return -EPERM;
> + if (ip->i_d.di_version >= 3 && (di_flags2 & XFS_DIFLAG2_COWEXTSIZE) &&
> + ip->i_d.di_cowextsize != fa->fsx_cowextsize >> mp->m_sb.sb_blocklog)
> + return -EPERM;
> +
> + return 0;
> +}
> +
> static int
> xfs_ioctl_setattr_xflags(
> struct xfs_trans *tp,
> @@ -1030,7 +1064,9 @@ xfs_ioctl_setattr_xflags(
> struct fsxattr *fa)
> {
> struct xfs_mount *mp = ip->i_mount;
> + uint16_t di_flags;
> uint64_t di_flags2;
> + int error;
>
> /* Can't change realtime flag if any extents are allocated. */
> if ((ip->i_d.di_nextents || ip->i_delayed_blks) &&
> @@ -1061,12 +1097,18 @@ xfs_ioctl_setattr_xflags(
> !capable(CAP_LINUX_IMMUTABLE))
> return -EPERM;
>
> - /* diflags2 only valid for v3 inodes. */
> + /* Don't allow changes to an immutable inode. */
> + di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
> di_flags2 = xfs_flags2diflags2(ip, fa->fsx_xflags);
> + error = xfs_ioctl_setattr_immutable(ip, fa, di_flags, di_flags2);
> + if (error)
> + return error;
> +
> + /* diflags2 only valid for v3 inodes. */
> if (di_flags2 && ip->i_d.di_version < 3)
> return -EINVAL;
>
> - ip->i_d.di_flags = xfs_flags2diflags(ip, fa->fsx_xflags);
> + ip->i_d.di_flags = di_flags;
> ip->i_d.di_flags2 = di_flags2;
>
> xfs_diflags_to_linux(ip);
