xfs
[Top] [All Lists]

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

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: Fix integer overflow in fs/xfs/linux-2.6/xfs_ioctl*.c
From: wzt wzt <wzt.wzt@xxxxxxxxx>
Date: Wed, 17 Mar 2010 11:00:54 +0800
Cc: linux-kernel@xxxxxxxxxxxxxxx, xfs-masters@xxxxxxxxxxx, xfs@xxxxxxxxxxx, aelder@xxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=g9QxQoOxjeH6Qn6BZrI8M9/YpRSp+roPK0/QPm2j7Zs=; b=owbM8nQS/oqVj5oDps+ofB744TgBQ1Cp3O8ipQBBt5aj0tYWuyThDiT0hsp6AoTs9q YMsDVIRRJocZ/L9JgrPrzyUnFDpRiy4CwkqcsrSmTT+OEVSWGHmt9SHHwa+bzaAX8uuS OZkY3/d2VwdVlYkhCKfryNlqcEq1HJhhf8Tds=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=Q1ndb9ge2iMxGIjdRMoobrjU9BKRlGICaonByq2sQgRx04dmYCrKwYjJcM15qCE2a6 uJpFqW4o75v93SFVnDcSMi/ZY7S4MgI6VaIYN0ycnH9ZTHyyGiKM7g2r7RAoVpQBZEls Z1lsT9pdjCPlnGdKx/vFiV0R24/GZXdXYiHZo=
In-reply-to: <20100317024101.GH12369@dastard>
References: <20100316155350.GB18579@xxxxxxxxxxxxxxxxxxxxx> <20100317024101.GH12369@dastard>
Thanks for your help, i'll send a new patch soon.

On Wed, Mar 17, 2010 at 10:41 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> On Tue, Mar 16, 2010 at 11:53:50PM +0800, wzt.wzt@xxxxxxxxx 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@xxxxxxxxxxxxxxx>
>>
>> ---
>>  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@xxxxxxxxxxxxx
>

<Prev in Thread] Current Thread [Next in Thread>