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

[Dwight Engen <dwight.engen@oracle.com>]

On Wed, 24 Jul 2013 10:27:43 -0400
Brian Foster <bfoster@redhat.com> 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@oracle.com>
> > ---
> >  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);
> >  	}
> >  
> > 
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs