* [PATCH] xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c
@ 2010-03-16 15:53 wzt.wzt
2010-03-17 2:41 ` Dave Chinner
0 siblings, 1 reply; 3+ messages in thread
From: wzt.wzt @ 2010-03-16 15:53 UTC (permalink / raw)
To: linux-kernel; +Cc: xfs-masters, aelder, xfs
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;
...
}
Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>
---
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(-)
diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
index 4ea1ee1..b05b3b7 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl.c
@@ -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);
diff --git a/fs/xfs/linux-2.6/xfs_ioctl32.c b/fs/xfs/linux-2.6/xfs_ioctl32.c
index 0bf6d61..7b8673e 100644
--- a/fs/xfs/linux-2.6/xfs_ioctl32.c
+++ b/fs/xfs/linux-2.6/xfs_ioctl32.c
@@ -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);
--
1.6.5.3
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c
2010-03-16 15:53 [PATCH] xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c wzt.wzt
@ 2010-03-17 2:41 ` Dave Chinner
2010-03-17 3:00 ` wzt wzt
0 siblings, 1 reply; 3+ messages in thread
From: Dave Chinner @ 2010-03-17 2:41 UTC (permalink / raw)
To: wzt.wzt; +Cc: xfs-masters, aelder, linux-kernel, xfs
On Tue, Mar 16, 2010 at 11:53:50PM +0800, wzt.wzt@gmail.com 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;
> ...
> }
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.
>
> Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>
>
> ---
> 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(-)
>
> diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
> index 4ea1ee1..b05b3b7 100644
> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
> @@ -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.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c
2010-03-17 2:41 ` Dave Chinner
@ 2010-03-17 3:00 ` wzt wzt
0 siblings, 0 replies; 3+ messages in thread
From: wzt wzt @ 2010-03-17 3:00 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs-masters, aelder, linux-kernel, xfs
Thanks for your help, i'll send a new patch soon.
On Wed, Mar 17, 2010 at 10:41 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Tue, Mar 16, 2010 at 11:53:50PM +0800, wzt.wzt@gmail.com 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;
>> ...
>> }
>
> 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.
>
>
>>
>> Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com>
>>
>> ---
>> 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(-)
>>
>> diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c
>> index 4ea1ee1..b05b3b7 100644
>> --- a/fs/xfs/linux-2.6/xfs_ioctl.c
>> +++ b/fs/xfs/linux-2.6/xfs_ioctl.c
>> @@ -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.
> --
> Dave Chinner
> david@fromorbit.com
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-03-17 2:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-16 15:53 [PATCH] xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c wzt.wzt
2010-03-17 2:41 ` Dave Chinner
2010-03-17 3:00 ` wzt wzt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox