Re: [PATCH] xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c

[wzt wzt <wzt.wzt@gmail.com>]

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