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

[Brian Foster <bfoster@redhat.com>]

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.

> 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..?

Brian

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

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