xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c
PATCH 1/1 xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c
STATIC int
xfs_compat_attrmulti_by_handle(
struct file *parfilp,
void __user *arg)
{
...
if (copy_from_user(&am_hreq, arg,
sizeof(compat_xfs_fsop_attrmulti_handlereq_t)))
return -XFS_ERROR(EFAULT);
...
error = E2BIG;
/* Not check the am_hreq.opcount max value from userspace,
m_hreq.opcount * sizeof(compat_xfs_attr_multiop_t) can make
integer overflow, and the if condition can be bypass. Though,
it can not make security problem, but fix it maybe better. */
size = am_hreq.opcount * sizeof(compat_xfs_attr_multiop_t);
if (!size || size > 16 * PAGE_SIZE)
goto out_dput;...
}
--- fs/xfs/linux-2.6/xfs_ioctl.c | 4 ++++ fs/xfs/linux-2.6/xfs_ioctl32.c | 4 ++++ 2 files changed, 8 insertions(+), 0 deletions(-)
fs/xfs/linux-2.6/xfs_ioctl.c +4 -0
@@ -526,6 +526,10 @@ xfs_attrmulti_by_handle(if (copy_from_user (&am_hreq, arg,sizeof (xfs_fsop_attrmulti_handlereq_t)))return -XFS_ERROR (EFAULT ); +/* overflow check */ +if (am_hreq.opcount >=INT_MAX /sizeof (xfs_attr_multiop_t)) +return -ENOMEM ; + dentry =xfs_handlereq_to_dentry (parfilp, &am_hreq.hreq);if (IS_ERR (dentry))return PTR_ERR (dentry);
fs/xfs/linux-2.6/xfs_ioctl32.c +4 -0
@@ -419,6 +419,10 @@ xfs_compat_attrmulti_by_handle( sizeof(compat_xfs_fsop_attrmulti_handlereq_t)))return -XFS_ERROR (EFAULT ); +/* overflow check */ +if (am_hreq.opcount >=INT_MAX /sizeof (compat_xfs_attr_multiop_t)) +return -ENOMEM ; + dentry =xfs_compat_handlereq_to_dentry (parfilp, &am_hreq.hreq);if (IS_ERR (dentry))return PTR_ERR (dentry);
Signature · 6 lines
1.6.5.3 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 18 lines · On Tue, Mar 16, 2010 at 11:53:50PM +0800, [email protected] wrote:
STATIC int
xfs_compat_attrmulti_by_handle(
struct file *parfilp,
void__user*arg)
{
...
if (copy_from_user(&am_hreq, arg,
sizeof(compat_xfs_fsop_attrmulti_handlereq_t)))
return -XFS_ERROR(EFAULT);
...error = E2BIG; /* Not check the am_hreq.opcount max value from userspace, m_hreq.opcount * sizeof(compat_xfs_attr_multiop_t) can make integer overflow, and the if condition can be bypass. Though, it can not make security problem, but fix it maybe better. */ size = am_hreq.opcount * sizeof(compat_xfs_attr_multiop_t); if (!size || size > 16 * PAGE_SIZE) goto out_dput;
Quoted 2 lines
...
}
This description could use a little work. ;)
Perhaps something like:
The am_hreq.opcount field in the xfs_attrmulti_by_handle() interface is not bounded correctly. The opcount is used to determine the size of the buffer required. The size is bounded, but can overflow and so the size checks may not be sufficient to catch invalid opcounts. Fix it by catching opcount values that would cause overflows before calculating the size.
Quoted 13 lines
Signed-off-by Zhitong Wang <[email protected]>--- fs/xfs/linux-2.6/xfs_ioctl.c | 4 ++++ fs/xfs/linux-2.6/xfs_ioctl32.c | 4 ++++ 2 files changed, 8 insertions(+), 0 deletions(-)fs/xfs/linux-2.6/xfs_ioctl.c +0
-0
fs/xfs/linux-2.6/xfs_ioctl.c xfs_attrmulti_by_handle( · line 532
Quoted 7 lines
fs/xfs/linux-2.6/xfs_ioctl.c +4
-0@@ -526,6 +526,10 @@ xfs_attrmulti_by_handle(if (copy_from_user (&am_hreq, arg,sizeof (xfs_fsop_attrmulti_handlereq_t)))return -XFS_ERROR (EFAULT ); +/* overflow check */ +if (am_hreq.opcount >=INT_MAX /sizeof (xfs_attr_multiop_t)) +return -ENOMEM ; +
The code currently return E2BIG for an opcount that is too large. I think this should also return E2BIG rather then ENOMEM. Does that seem reasonable?
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Thanks for your help, i'll send a new patch soon.
Quoted 59 lines · Dave Chinner
Quoted 12 lines · On Tue, Mar 16, 2010 at 11:53:50PM +0800, [email protected] wrote:
STATIC int
xfs_compat_attrmulti_by_handle(
struct file *parfilp,
void __user *arg)
{
...
if (copy_from_user(&am_hreq, arg, sizeof(compat_xfs_fsop_attrmulti_handlereq_t))) return -XFS_ERROR(EFAULT);
...
error = E2BIG;
/* Not check the am_hreq.opcount max value from userspace,Quoted 8 lines
m_hreq.opcount *
sizeof(compat_xfs_attr_multiop_t)can make integer overflow, and the if condition can be bypass. Though, it can not make security problem, but fix it maybe better. */ size = am_hreq.opcount *sizeof(compat_xfs_attr_multiop_t); if (!size || size > 16 *PAGE_SIZE)
gotoout_dput;
...
}This description could use a little work. ;)
Perhaps something like:
The am_hreq.opcount field in the
xfs_attrmulti_by_handle()interface is not bounded correctly. The opcount is used to determine the size of the buffer required. The size is bounded, but can overflow and so the size checks may not be sufficient to catch invalid opcounts. Fix it by catching opcount values that would cause overflows before calculating the size.Quoted 13 lines
Signed-off-by Zhitong Wang <[email protected]>---
fs/xfs/linux-2.6/xfs_ioctl.c | 4 ++++
fs/xfs/linux-2.6/xfs_ioctl32.c | 4 ++++2 files changed, 8 insertions(+), 0 deletions(-)fs/xfs/linux-2.6/xfs_ioctl.c +0
-0fs/xfs/linux-2.6/xfs_ioctl.c xfs_attrmulti_by_handle( · line 532
Quoted 7 lines
fs/xfs/linux-2.6/xfs_ioctl.c +4
-0@@ -526,6 +526,10 @@ xfs_attrmulti_by_handle( if (copy_from_user(&am_hreq, arg, sizeof(xfs_fsop_attrmulti_handlereq_t))) return -XFS_ERROR(EFAULT); + /* overflow check */ + if (am_hreq.opcount >= INT_MAX / sizeof(xfs_attr_multiop_t)) + return -ENOMEM; +The code currently return E2BIG for an opcount that is too large. I think this should also return E2BIG rather then ENOMEM. Does that seem reasonable?
Quoted 7 lines
Cheers,
Dave.
--
Dave Chinner
[email protected]
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs