xfs
[Top] [All Lists]

Re: [PATCH v5 6/7] xfs: add permission check to free eofblocks ioctl

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH v5 6/7] xfs: add permission check to free eofblocks ioctl
From: Dwight Engen <dwight.engen@xxxxxxxxxx>
Date: Wed, 24 Jul 2013 12:22:07 -0400
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <51EFE45F.4000801@xxxxxxxxxx>
Organization: Oracle Corporation
References: <20130724005301.0ed718d2@xxxxxxxxxx> <51EFE45F.4000801@xxxxxxxxxx>
On Wed, 24 Jul 2013 10:27:43 -0400
Brian Foster <bfoster@xxxxxxxxxx> wrote:

> On 07/24/2013 12:53 AM, Dwight Engen wrote:
> > We need to check that userspace callers can only truncate
> > preallocated blocks from files they have write access to to prevent
> > them from prematurley reclaiming blocks from another user. The
> > internal reclaimer will not specify the XFS_EOF_FLAGS_PERM_CHECK
> > flag, but userspace callers should.
> > 
> > Add check for read-only filesystem to free eofblocks ioctl.
> > 
> > Signed-off-by: Dwight Engen <dwight.engen@xxxxxxxxxx>
> > ---
> >  fs/xfs/xfs_fs.h     | 1 +
> >  fs/xfs/xfs_icache.c | 4 ++++
> >  fs/xfs/xfs_ioctl.c  | 4 ++++
> >  3 files changed, 9 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
> > index 7eb4a5e..aee4b12 100644
> > --- a/fs/xfs/xfs_fs.h
> > +++ b/fs/xfs/xfs_fs.h
> > @@ -361,6 +361,7 @@ struct xfs_fs_eofblocks {
> >  #define XFS_EOF_FLAGS_GID          (1 << 2) /* filter by gid
> > */ #define XFS_EOF_FLAGS_PRID               (1 << 3) /* filter by
> > project id */ #define XFS_EOF_FLAGS_MINFILESIZE     (1 << 4) /*
> > filter by min file size */ +#define XFS_EOF_FLAGS_PERM_CHECK
> > (1 << 5) /* check can write inode */ #define
> > XFS_EOF_FLAGS_VALID \ (XFS_EOF_FLAGS_SYNC | \
> >      XFS_EOF_FLAGS_UID |    \
> 
> We're not updating the VALID definition, which means the ioctl() would
> fail if the caller sets this flag. I find that a little confusing
> since we're effectively enforcing it. Given that the new flag would be
> exported, it might be a better idea to add it to the valid definition
> even though we don't require the caller to set it.
> 
> An alternative might be to duplicate the set of flags in xfs_icache.h
> and not export this one at all, but I don't know it's really worth
> that.

I didn't put it in VALID because its really an internal flag, and we
don't want userspace to think that we will honor them specifying it
or not. ie. its not a valid bit for them to turn on. I agree it would be
best not to export it though, how about if we move the definition to
xfs_icache.h with a guard against someone accidentally adding a new
duplicate bit in xfs_fs.h, like this:

#define XFS_EOF_FLAGS_PERM_CHECK        (1 << 5) /* check can write inode */
#if XFS_EOF_FLAGS_PERM_CHECK & XFS_EOF_FLAGS_VALID
#error "Internal XFS_EOF_FLAGS_PERM_CHECK duplicated bit from 
XFS_EOF_FLAGS_VALID"
#endif

Maybe since this is internal we should also start at 1<<31 to allow
room for exported flags to grow?

> > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > index ed35584..823f2c0 100644
> > --- a/fs/xfs/xfs_icache.c
> > +++ b/fs/xfs/xfs_icache.c
> > @@ -1247,6 +1247,10 @@ xfs_inode_free_eofblocks(
> >             if (!xfs_inode_match_id(ip, eofb))
> >                     return 0;
> >  
> > +           if ((eofb->eof_flags & XFS_EOF_FLAGS_PERM_CHECK) &&
> > +               inode_permission(VFS_I(ip), MAY_WRITE))
> > +                   return 0;
> > +
> >             /* skip the inode if the file size is too small */
> >             if (eofb->eof_flags & XFS_EOF_FLAGS_MINFILESIZE &&
> >                 XFS_ISIZE(ip) < eofb->eof_min_file_size)
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index ecab261..c7cb632 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1613,6 +1613,9 @@ xfs_file_ioctl(
> >             struct xfs_fs_eofblocks eofb;
> >             struct xfs_eofblocks keofb;
> >  
> > +           if (IS_RDONLY(inode))
> > +                   return -XFS_ERROR(EROFS);
> > +
> >             if (copy_from_user(&eofb, arg, sizeof(eofb)))
> >                     return -XFS_ERROR(EFAULT);
> >  
> > @@ -1630,6 +1633,7 @@ xfs_file_ioctl(
> >             if (error)
> >                     return -error;
> >  
> > +           keofb.eof_flags |= XFS_EOF_FLAGS_PERM_CHECK;
> 
> And perhaps this should also be in the new helper..?

Okay, yep I can move this and the other struct xfs_fs_eofblocks checks
you mentioned into the _from_user() helper.

> Brian
> 
> >             return -xfs_icache_free_eofblocks(mp, &keofb);
> >     }
> >  
> > 
> 

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