linux-kernel - Re: [PATCH] xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 17 Mar 2010 11:00:54 +0800
From:	wzt wzt <wzt.wzt@...il.com>
To:	Dave Chinner <david@...morbit.com>
Cc:	linux-kernel@...r.kernel.org, xfs-masters@....sgi.com,
	xfs@....sgi.com, aelder@....com
Subject: Re: [PATCH] xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c

Thanks for your help, i'll send a new patch soon.

On Wed, Mar 17, 2010 at 10:41 AM, Dave Chinner <david@...morbit.com> wrote:
> On Tue, Mar 16, 2010 at 11:53:50PM +0800, wzt.wzt@...il.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@...baba-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@...morbit.com
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/