[v2,4/4] xfs: don't allow most setxattr to immutable files
diff mbox series

Message ID 20190409031929.GE5147@magnolia
State New
Headers show
Series
  • Untitled series #102859
Related show

Commit Message

Darrick J. Wong April 9, 2019, 3:19 a.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

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@oracle.com>
---
 fs/xfs/xfs_ioctl.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

Comments

Amir Goldstein April 9, 2019, 8:24 a.m. UTC | #1
On Tue, Apr 9, 2019 at 6:19 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> 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@oracle.com>
> ---
>  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);
Darrick J. Wong April 17, 2019, 7:01 p.m. UTC | #2
On Tue, Apr 09, 2019 at 11:24:10AM +0300, Amir Goldstein wrote:
> On Tue, Apr 9, 2019 at 6:19 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > 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@oracle.com>
> > ---
> >  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.

However, the API does allow callers to clear immutable and set other
fields in one go, so just because xfs_io won't do it doesn't mean we can
ignore it.

Then again I guess the manpage doesn't explicitly say what the behavior
is supposed to be, so I guess we'll just ... argh, fine I'll go fix the
manpage to document the behavior.

--D

> 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);

Patch
diff mbox series

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.  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);