xfs: add capable check to free eofblocks ioctl
PATCH v7 6/7 xfs: add capable check to free eofblocks ioctl
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.
--- fs/xfs/xfs_ioctl.c | 6 ++++++ 1 file changed, 6 insertions(+)
fs/xfs/xfs_ioctl.c +6 -0
@@ -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 );
Signature · 6 lines
1.8.1.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 19 lines · Dwight Engen
Check for
CAP_SYS_ADMINsince 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 useinode_permissionto 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 <[email protected]>--- fs/xfs/xfs_ioctl.c | 6 ++++++ 1 file changed, 6 insertions(+)fs/xfs/xfs_ioctl.c +0
-0
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1618
Quoted 6 lines
fs/xfs/xfs_ioctl.c +3
-0@@ -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).
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1621
Quoted 3 lines
fs/xfs/xfs_ioctl.c +3
-0+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
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1624
Quoted 4 lines
fs/xfs/xfs_ioctl.c +0
-0if (copy_from_user (&eofb, arg,sizeof (eofb)))return -XFS_ERROR (EFAULT );
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 23 lines · Brian Foster
Quoted 22 lines · Dwight Engen
Check for
CAP_SYS_ADMINsince 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 useinode_permissionto 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 <[email protected]>--- fs/xfs/xfs_ioctl.c | 6 ++++++ 1 file changed, 6 insertions(+)fs/xfs/xfs_ioctl.c +0
-0@@ -1613,6 +1613,12 @@ xfs_file_ioctl(struct xfs_fs_eofblocks eofb;struct xfs_eofblocks keofb;
Quoted 12 lines
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1618
Quoted 3 lines
fs/xfs/xfs_ioctl.c +3
-0+if (!capable (CAP_SYS_ADMIN )) +return -EPERM ; +I see that we aren't using
XFS_ERROR()on the EPERM returns inxfs_file_ioctl(), but I see it used for other capability checks elsewhere (i.e., down inxfs_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 aBUG()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.
Quoted 10 lines
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1621
Quoted 4 lines
fs/xfs/xfs_ioctl.c +3
-0+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_mountstructure: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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 20 lines · Dwight Engen
Check for
CAP_SYS_ADMINsince 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 useinode_permissionto 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 <[email protected]>--- fs/xfs/xfs_ioctl.c | 6 ++++++ 1 file changed, 6 insertions(+)fs/xfs/xfs_ioctl.c +0
-0@@ -1613,6 +1613,12 @@ xfs_file_ioctl(struct xfs_fs_eofblocks eofb;
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1623
Quoted 10 lines
fs/xfs/xfs_ioctl.c +6
-0struct 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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 11 lines · Dwight Engen
Check for
CAP_SYS_ADMINsince 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 useinode_permissionto 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 <[email protected]>---
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs