check for invalid flags in xfs_attrlist_by_handle
PATCH 1/1 check for invalid flags in xfs_attrlist_by_handle
xfs_attrlist_by_handle should only take the ATTR_ flags for the root namespaces. The ATTR_KERN* flags may change at anytime and expect special preconditions that can't be guaranteed for userspace-originating requests. For example passing down ATTR_KERNNOVAL through xfs_attrlist_by_handle will hit an assert in debug builds currently.
fs/xfs/linux-2.6/xfs_ioctl.c +6 -0
@@ -470,6 +470,12 @@ xfs_attrlist_by_handle(if (al_hreq.buflen >XATTR_LIST_MAX )return -XFS_ERROR (EINVAL ); +/* +* Reject flags, only allow namespaces. +*/ +if (al_hreq.flags & ~(ATTR_ROOT |ATTR_TRUST |ATTR_SECURE )) +return -XFS_ERROR (EINVAL ); + error =xfs_vget_fsop_handlereq (mp, parinode, &al_hreq.hreq, &inode);if (error) goto out;
Fair enough.
Actually, I think we only use ATTR_ROOT and ATTR_SECURE for the namespace flags.
So you could probably use: XFS_ATTR_NSP_ARGS
xfs_attr_leaf.h:#define XFS_ATTR_NSP_ARGS_MASK (ATTR_ROOT | ATTR_SECURE)
xfs_attr_leaf.h:#define XFS_ATTR_NSP_ARGS(flags) ((flags) & XFS_ATTR_NSP_ARGS_MASK) and something like:
if (!XFS_ATTR_NSP_ARGS(al_hreq.flags))
return -XFS_ERROR(EINVAL);
Though would probably then need to include the right header (xfs_attr_leaf.h) for it...
--Tim
Quoted 27 lines · Christoph Hellwig
xfs_attrlist_by_handleshould only take theATTR_flags for the root namespaces. The ATTR_KERN* flags may change at anytime and expect special preconditions that can't be guaranteed for userspace-originating requests. For example passing downATTR_KERNNOVALthroughxfs_attrlist_by_handlewill hit an assert in debug builds currently.Signed-off-by Christoph Hellwig <[email protected]>Index:
linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================fs/xfs/linux-2.6/xfs_ioctl.c +6
-0@@ -470,6 +470,12 @@ xfs_attrlist_by_handle(if (al_hreq.buflen >XATTR_LIST_MAX )return -XFS_ERROR (EINVAL ); +/* +* Reject flags, only allow namespaces. +*/ +if (al_hreq.flags & ~(ATTR_ROOT |ATTR_TRUST |ATTR_SECURE )) +return -XFS_ERROR (EINVAL ); + error =xfs_vget_fsop_handlereq (mp, parinode, &al_hreq.hreq, &inode);if (error) goto out;
Quoted 8 lines · Timothy Shimmin
Fair enough.
Actually, I think we only useATTR_ROOTandATTR_SECUREfor the namespace flags.
So you could probably use:XFS_ATTR_NSP_ARGS
xfs_attr_leaf.h:#defineXFS_ATTR_NSP_ARGS_MASK(ATTR_ROOT|ATTR_SECURE)
xfs_attr_leaf.h:#defineXFS_ATTR_NSP_ARGS(flags)((flags) &XFS_ATTR_NSP_ARGS_MASK) and something like:
Quoted 4 lines
if (!XFS_ATTR_NSP_ARGS(al_hreq.flags))
return -XFS_ERROR(EINVAL);Though would probably then need to include the right header (
xfs_attr_leaf.h) for it...
Makes sense. I'll revise the patch and send an updated version after running it through QA.
Quoted 10 lines · Timothy Shimmin
Fair enough.
Actually, I think we only useATTR_ROOTandATTR_SECUREfor the namespace flags.
So you could probably use:XFS_ATTR_NSP_ARGS
xfs_attr_leaf.h:#defineXFS_ATTR_NSP_ARGS_MASK(ATTR_ROOT|ATTR_SECURE)
xfs_attr_leaf.h:#defineXFS_ATTR_NSP_ARGS(flags)((flags) &XFS_ATTR_NSP_ARGS_MASK) and something like:if (!XFS_ATTR_NSP_ARGS(al_hreq.flags))
return -XFS_ERROR(EINVAL);
Actually a zero flags is of course valid too.
So the check should be & ~(ATTR_ROOT | ATTR_SECURE). I could use XFS_ATTR_NSP_ARGS_MASK but that would pull in not just xfs_attr_leaf.h but also xfs_da_btree.h and that needs even more headers..
So I propose this simple version:
fs/xfs/linux-2.6/xfs_ioctl.c +6 -0
@@ -470,6 +470,12 @@ xfs_attrlist_by_handle(if (al_hreq.buflen >XATTR_LIST_MAX )return -XFS_ERROR (EINVAL ); +/* +* Reject flags, only allow namespaces. +*/ +if (al_hreq.flags & ~(ATTR_ROOT |ATTR_SECURE )) +return -XFS_ERROR (EINVAL ); + error =xfs_vget_fsop_handlereq (mp, parinode, &al_hreq.hreq, &inode);if (error) goto out;
Quoted 9 lines · Christoph Hellwig
Quoted 8 lines · Timothy Shimmin
Fair enough.
Actually, I think we only useATTR_ROOTandATTR_SECUREfor the namespace flags.
So you could probably use:XFS_ATTR_NSP_ARGS
xfs_attr_leaf.h:#defineXFS_ATTR_NSP_ARGS_MASK(ATTR_ROOT|ATTR_SECURE)
xfs_attr_leaf.h:#defineXFS_ATTR_NSP_ARGS(flags)((flags) &XFS_ATTR_NSP_ARGS_MASK) and something like:
Quoted 5 lines
Quoted 2 lines
if (!XFS_ATTR_NSP_ARGS(al_hreq.flags))
return -XFS_ERROR(EINVAL);Actually a zero flags is of course valid too.
Ah, that would be why I used the phrase "something like" ;-)) Good pt.
Quoted 6 lines
So the check should be & ~(ATTR_ROOT |
ATTR_SECURE). I could useXFS_ATTR_NSP_ARGS_MASKbut that would pull in not justxfs_attr_leaf.hbut alsoxfs_da_btree.hand that needs even more headers..So I propose this simple version:
Cool.
I was just thinking about centralising stuff in case we extend the namespaces. But that's fine.
I'll check it in...
--Tim
Quoted 19 lines
Index:
linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================fs/xfs/linux-2.6/xfs_ioctl.c +6
-0@@ -470,6 +470,12 @@ xfs_attrlist_by_handle(if (al_hreq.buflen >XATTR_LIST_MAX )return -XFS_ERROR (EINVAL ); +/* +* Reject flags, only allow namespaces. +*/ +if (al_hreq.flags & ~(ATTR_ROOT |ATTR_SECURE )) +return -XFS_ERROR (EINVAL ); + error =xfs_vget_fsop_handlereq (mp, parinode, &al_hreq.hreq, &inode);if (error) goto out;