linux-kernel - Re: [PATCH v1 1/1] USB: serial: Add boundry check for read

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <0055f93b-8497-5dfc-4233-9cc72bf690fc@linux.intel.com>
Date:   Wed, 7 Mar 2018 13:41:51 -0800
From:   sathyanarayanan kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     johan@...nel.org, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] USB: serial: Add boundry check for read_urbs array
 access



On 03/07/2018 12:58 PM, Greg KH wrote:
> On Wed, Mar 07, 2018 at 12:23:56PM -0800, sathyanarayanan.kuppuswamy@...ux.intel.com wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
>>
>> In usb_serial_generic_submit_read_urb() function we are accessing the
>> port->read_urbs array without any boundry checks. This might lead to
>> kernel panic when index value goes above array length.
>>
>> One posible call path for this issue is,
>>
>> usb_serial_generic_read_bulk_callback()
>> {
>>   ...
>>   if (!port->throttled) {
>> 	usb_serial_generic_submit_read_urb(port, i, GFP_ATOMIC);
>>   ...
>> }
> How does i ever get to be greater than the array size here in this
> function?  It directly came from looking in that array in the first
> place :)
>
> So I don't see why your check is needed, what other code path would ever
> call this function in a way that the bounds check would be needed?
void usb_serial_generic_read_bulk_callback(struct urb *urb)

385         for (i = 0; i < ARRAY_SIZE(port->read_urbs); ++i) {
386                 if (urb == port->read_urbs[i])
387                         break;
388         }

In here, after this for loop is done (without any matching urb), i value 
will be equal to ARRAY_SIZE(port->read_urbs). So there is a possibility 
of usb_serial_generic_submit_read_urb() getting called with this invalid 
index.

>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer