xfs: add permission check to free eofblocks ioctl
PATCH v5 6/7 xfs: add permission check to free eofblocks ioctl
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.
--- fs/xfs/xfs_fs.h | 1 + fs/xfs/xfs_icache.c | 4 ++++ fs/xfs/xfs_ioctl.c | 4 ++++ 3 files changed, 9 insertions(+)
fs/xfs/xfs_fs.h +1 -0
@@ -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 | \
fs/xfs/xfs_icache.c +4 -0
@@ -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)
fs/xfs/xfs_ioctl.c +4 -0
@@ -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 ;return -xfs_icache_free_eofblocks (mp, &keofb); }
Signature · 6 lines
1.8.1.4 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 19 lines · Dwight Engen
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_CHECKflag, but userspace callers should.Add check for read-only filesystem to free eofblocks ioctl.
Signed-off-by Dwight Engen <[email protected]>--- fs/xfs/xfs_fs.h | 1 + fs/xfs/xfs_icache.c | 4 ++++ fs/xfs/xfs_ioctl.c | 4 ++++ 3 files changed, 9 insertions(+)fs/xfs/xfs_fs.h +0
-0
fs/xfs/xfs_fs.h struct xfs_fs_eofblocks { · line 367
Quoted 7 lines
fs/xfs/xfs_fs.h +1
-0@@ -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.
Quoted 30 lines
fs/xfs/xfs_icache.c +4
-0@@ -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)fs/xfs/xfs_ioctl.c +3
-0@@ -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 );
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1636
Quoted 4 lines
fs/xfs/xfs_ioctl.c +1
-0@@ -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
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1639
Quoted 4 lines
fs/xfs/xfs_ioctl.c +0
-0return -xfs_icache_free_eofblocks (mp, &keofb); }
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 35 lines · On Wed, 24 Jul 2013 10:27:43 -0400 Brian Foster <[email protected]> wrote:
Quoted 22 lines · Dwight Engen
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_CHECKflag, but userspace callers should.Add check for read-only filesystem to free eofblocks ioctl.
Signed-off-by Dwight Engen <[email protected]>--- fs/xfs/xfs_fs.h | 1 + fs/xfs/xfs_icache.c | 4 ++++ fs/xfs/xfs_ioctl.c | 4 ++++ 3 files changed, 9 insertions(+)fs/xfs/xfs_fs.h +0
-0@@ -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 Quoted 5 lines
project id */ #define
XFS_EOF_FLAGS_MINFILESIZE(1 << 4) /*
filter by min file size */ +#defineXFS_EOF_FLAGS_PERM_CHECK(1 << 5) /* check can write inode */ #defineXFS_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.
Quoted 3 lines
An alternative might be to duplicate the set of flags in
xfs_icache.hand 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?
Quoted 29 lines
Quoted 29 lines
fs/xfs/xfs_icache.c +4
-0@@ -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)fs/xfs/xfs_ioctl.c +3
-0@@ -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 );
Quoted 7 lines
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1636
Quoted 5 lines
fs/xfs/xfs_ioctl.c +1
-0@@ -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.
Quoted 7 lines
Brian
Quoted 4 lines
fs/xfs/xfs_ioctl.c +0
-0return -xfs_icache_free_eofblocks (mp, &keofb); }
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 49 lines · Dwight Engen
Quoted 35 lines · On Wed, 24 Jul 2013 10:27:43 -0400 Brian Foster <[email protected]> wrote:
Quoted 22 lines · Dwight Engen
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_CHECKflag, but userspace callers should.Add check for read-only filesystem to free eofblocks ioctl.
Signed-off-by Dwight Engen <[email protected]>--- fs/xfs/xfs_fs.h | 1 + fs/xfs/xfs_icache.c | 4 ++++ fs/xfs/xfs_ioctl.c | 4 ++++ 3 files changed, 9 insertions(+)fs/xfs/xfs_fs.h +0
-0@@ -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 Quoted 5 lines
project id */ #define
XFS_EOF_FLAGS_MINFILESIZE(1 << 4) /*
filter by min file size */ +#defineXFS_EOF_FLAGS_PERM_CHECK(1 << 5) /* check can write inode */ #defineXFS_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.Quoted 3 lines
An alternative might be to duplicate the set of flags in
xfs_icache.hand 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.hwith a guard against someone accidentally adding a new duplicate bit inxfs_fs.h, like this:
Quoted 7 lines
#define
XFS_EOF_FLAGS_PERM_CHECK(1 << 5) /* check can write inode */
#ifXFS_EOF_FLAGS_PERM_CHECK&XFS_EOF_FLAGS_VALID
#error "InternalXFS_EOF_FLAGS_PERM_CHECKduplicated bit fromXFS_EOF_FLAGS_VALID" #endifMaybe since this is internal we should also start at 1<<31 to allow room for exported flags to grow?
Fair enough. It sounds reasonable to me to start a separate, internal only set of flags. I'm not sure if you're suggesting to only use the msb, or start at the msb in decreasing fashion.
I was going to suggest reserving the last byte for internal flags or perhaps use a u64 for the internal eofb flags and reserve bits 32-63 for internal use (or just create another variable). I also think a slight name change is useful to differentiate the internal flags. Perhaps XFS_KEOF_*?
Brian
Quoted 34 lines
Quoted 34 lines
Quoted 34 lines
fs/xfs/xfs_icache.c +4
-0@@ -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)fs/xfs/xfs_ioctl.c +3
-0@@ -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;
Quoted 15 lines
Quoted 3 lines
fs/xfs/xfs_ioctl.c xfs_file_ioctl( · line 1636
Quoted 1 line
fs/xfs/xfs_ioctl.c +1
-0+ 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_eofblockschecks you mentioned into the_from_user()helper.Quoted 7 lines
Brian
Quoted 4 lines
fs/xfs/xfs_ioctl.c +0
-0return -xfs_icache_free_eofblocks (mp, &keofb); }
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs