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

[wzt wzt]

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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/