* [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl
@ 2013-07-30 3:07 Dwight Engen
2013-07-30 12:33 ` Brian Foster
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Dwight Engen @ 2013-07-30 3:07 UTC (permalink / raw)
To: xfs
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl
2013-07-30 3:07 [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl Dwight Engen
@ 2013-07-30 12:33 ` Brian Foster
2013-07-30 23:27 ` Dave Chinner
2013-07-30 23:16 ` Dave Chinner
2013-07-31 7:14 ` Gao feng
2 siblings, 1 reply; 5+ messages in thread
From: Brian Foster @ 2013-07-30 12:33 UTC (permalink / raw)
To: Dwight Engen; +Cc: xfs
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl
2013-07-30 3:07 [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl Dwight Engen
2013-07-30 12:33 ` Brian Foster
@ 2013-07-30 23:16 ` Dave Chinner
2013-07-31 7:14 ` Gao feng
2 siblings, 0 replies; 5+ messages in thread
From: Dave Chinner @ 2013-07-30 23:16 UTC (permalink / raw)
To: Dwight Engen; +Cc: xfs
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl
2013-07-30 12:33 ` Brian Foster
@ 2013-07-30 23:27 ` Dave Chinner
0 siblings, 0 replies; 5+ messages in thread
From: Dave Chinner @ 2013-07-30 23:27 UTC (permalink / raw)
To: Brian Foster; +Cc: Dwight Engen, xfs
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl
2013-07-30 3:07 [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl Dwight Engen
2013-07-30 12:33 ` Brian Foster
2013-07-30 23:16 ` Dave Chinner
@ 2013-07-31 7:14 ` Gao feng
2 siblings, 0 replies; 5+ messages in thread
From: Gao feng @ 2013-07-31 7:14 UTC (permalink / raw)
To: Dwight Engen; +Cc: xfs
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-07-31 7:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-30 3:07 [PATCH v7 6/7] xfs: add capable check to free eofblocks ioctl Dwight Engen
2013-07-30 12:33 ` Brian Foster
2013-07-30 23:27 ` Dave Chinner
2013-07-30 23:16 ` Dave Chinner
2013-07-31 7:14 ` Gao feng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox