[PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl

[PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl

[Dwight Engen]

Check for CAP_SYS_ADMIN since the caller can truncate preallocated
blocks from files they do not own nor have write access to. A more
fine grained access check was considered: require the caller to
specify their own uid/gid and to use inode_permission to check for
write, but this would not catch the case of an inode not reachable
via path traversal from the callers mount namespace.

Add check for read-only filesystem to free eofblocks ioctl.

Signed-off-by: Dwight Engen <dwight.engen@oracle.com>
---
 fs/xfs/xfs_ioctl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 6e72eff..b1990ac 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1613,6 +1613,12 @@ xfs_file_ioctl(
 		struct xfs_fs_eofblocks eofb;
 		struct xfs_eofblocks keofb;
 
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+
+		if (IS_RDONLY(inode))
+			return -XFS_ERROR(EROFS);
+
 		if (copy_from_user(&eofb, arg, sizeof(eofb)))
 			return -XFS_ERROR(EFAULT);
 
-- 
1.8.1.4

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

Re: [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl

[Brian Foster]

On 07/29/2013 11:07 PM, Dwight Engen wrote:
> Check for CAP_SYS_ADMIN since the caller can truncate preallocated
> blocks from files they do not own nor have write access to. A more
> fine grained access check was considered: require the caller to
> specify their own uid/gid and to use inode_permission to check for
> write, but this would not catch the case of an inode not reachable
> via path traversal from the callers mount namespace.
> 
> Add check for read-only filesystem to free eofblocks ioctl.
> 
> Signed-off-by: Dwight Engen <dwight.engen@oracle.com>
> ---
>  fs/xfs/xfs_ioctl.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 6e72eff..b1990ac 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1613,6 +1613,12 @@ xfs_file_ioctl(
>  		struct xfs_fs_eofblocks eofb;
>  		struct xfs_eofblocks keofb;
>  
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EPERM;
> +

I see that we aren't using XFS_ERROR() on the EPERM returns in
xfs_file_ioctl(), but I see it used for other capability checks
elsewhere (i.e., down in xfs_growfs_data()). Perhaps somebody can chime
in as to the reasoning for that..? I guess it could be that we wouldn't
want to fire a BUG() at the interface point (ioctl()) on debug kernels
for every time a user attempts an operation they don't have the ability
to perform (e.g., we should notice on internal failures, not when
userspace sends us something wrong).

> +		if (IS_RDONLY(inode))
> +			return -XFS_ERROR(EROFS);
> +

This should probably be consistent with the other read-only checks in
the ioctl code and check the xfs_mount structure:

	if (mp->m_flags & XFS_MOUNT_RDONLY)
		return -XFS_ERROR(EROFS);

Brian

>  		if (copy_from_user(&eofb, arg, sizeof(eofb)))
>  			return -XFS_ERROR(EFAULT);
>  
> 

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

Re: [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl

[Dave Chinner]

On Tue, Jul 30, 2013 at 08:33:39AM -0400, Brian Foster wrote:
> On 07/29/2013 11:07 PM, Dwight Engen wrote:
> > Check for CAP_SYS_ADMIN since the caller can truncate preallocated
> > blocks from files they do not own nor have write access to. A more
> > fine grained access check was considered: require the caller to
> > specify their own uid/gid and to use inode_permission to check for
> > write, but this would not catch the case of an inode not reachable
> > via path traversal from the callers mount namespace.
> > 
> > Add check for read-only filesystem to free eofblocks ioctl.
> > 
> > Signed-off-by: Dwight Engen <dwight.engen@oracle.com>
> > ---
> >  fs/xfs/xfs_ioctl.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 6e72eff..b1990ac 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1613,6 +1613,12 @@ xfs_file_ioctl(
> >  		struct xfs_fs_eofblocks eofb;
> >  		struct xfs_eofblocks keofb;
> >  
> > +		if (!capable(CAP_SYS_ADMIN))
> > +			return -EPERM;
> > +
> 
> I see that we aren't using XFS_ERROR() on the EPERM returns in
> xfs_file_ioctl(), but I see it used for other capability checks
> elsewhere (i.e., down in xfs_growfs_data()). Perhaps somebody can chime
> in as to the reasoning for that..? I guess it could be that we wouldn't
> want to fire a BUG() at the interface point (ioctl()) on debug kernels
> for every time a user attempts an operation they don't have the ability
> to perform (e.g., we should notice on internal failures, not when
> userspace sends us something wrong).

Essentially.

The error trapping is used to pinpoint where an error first occurs.
e.g.  where a ENOMEM error is first detected. There might be tens of
memory allocations inside a transaction that is failing with ENOMEM
and shutting down, so tracking down where it came from is quite
hard. Hence trapping the first ENOMEM occurrence with a BUG() via
XFS_ERROR() will tell you exactly where it came from. Similarly,
some ioctls can return EINVAL for 20 different reasons (e.g.
xfs_swapext() path) and so trapping them can be useful if it's a
random failure that can't be captured otherwise.

Because all the EPERM checks are in the outer layers and generally
easy to spot, they are easy to test for and easy to track down when
they occur. Hence, in general, we don't need to trap them. 

> 
> > +		if (IS_RDONLY(inode))
> > +			return -XFS_ERROR(EROFS);
> > +
> 
> This should probably be consistent with the other read-only checks in
> the ioctl code and check the xfs_mount structure:
> 
> 	if (mp->m_flags & XFS_MOUNT_RDONLY)
> 		return -XFS_ERROR(EROFS);

Oh, I didn't even think about that - IS_RDONLY() actually checks the
superblock flag, so it's no different to checking the xfs_mount
flags. ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl

[Dave Chinner]

On Mon, Jul 29, 2013 at 11:07:05PM -0400, Dwight Engen wrote:
> Check for CAP_SYS_ADMIN since the caller can truncate preallocated
> blocks from files they do not own nor have write access to. A more
> fine grained access check was considered: require the caller to
> specify their own uid/gid and to use inode_permission to check for
> write, but this would not catch the case of an inode not reachable
> via path traversal from the callers mount namespace.
> 
> Add check for read-only filesystem to free eofblocks ioctl.
> 
> Signed-off-by: Dwight Engen <dwight.engen@oracle.com>
> ---
>  fs/xfs/xfs_ioctl.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 6e72eff..b1990ac 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1613,6 +1613,12 @@ xfs_file_ioctl(
>  		struct xfs_fs_eofblocks eofb;
>  		struct xfs_eofblocks keofb;
>  
> +		if (!capable(CAP_SYS_ADMIN))
> +			return -EPERM;
> +
> +		if (IS_RDONLY(inode))
> +			return -XFS_ERROR(EROFS);
> +
>  		if (copy_from_user(&eofb, arg, sizeof(eofb)))
>  			return -XFS_ERROR(EFAULT);

Looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl

[Gao feng]

On 07/30/2013 11:07 AM, Dwight Engen wrote:
> Check for CAP_SYS_ADMIN since the caller can truncate preallocated
> blocks from files they do not own nor have write access to. A more
> fine grained access check was considered: require the caller to
> specify their own uid/gid and to use inode_permission to check for
> write, but this would not catch the case of an inode not reachable
> via path traversal from the callers mount namespace.
> 
> Add check for read-only filesystem to free eofblocks ioctl.
> 
> Signed-off-by: Dwight Engen <dwight.engen@oracle.com>
> ---

Reviewed-by: Gao feng <gaofeng@cn.fujitsu.com>

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