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

Date:   Thu, 8 Mar 2018 16:34:44 -0800
From:   sathyanarayanan kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Oliver Neukum <oneukum@...e.com>, johan@...nel.org,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v1 1/1] USB: serial: Add boundry check for read_urbs array
 access



On 03/08/2018 03:43 PM, Greg KH wrote:
> On Thu, Mar 08, 2018 at 03:29:48PM -0800, sathyanarayanan kuppuswamy wrote:
>>
>> On 03/08/2018 12:54 AM, Oliver Neukum wrote:
>>> Am Mittwoch, den 07.03.2018, 13:41 -0800 schrieb sathyanarayanan
>>> kuppuswamy       :
>>>> On 03/07/2018 12:58 PM, Greg KH wrote:
>>>>> 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.
>>> If this happens the function was called for a stray URB.
>>> Your check comes to late. We have called set_bit with an invalid index
>>> and other shit.
>>> We definitely do not just want to return an error in that case.
>> In that case do you think we should use some WARN_ON() for invalid index in
>> usb_serial_generic_read_bulk_callback()?
> No, again, how could that ever happen?
>
> Don't add pointless error checking for things that are impossible to
> ever hit :)
Thanks Greg.
>
> 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