Re: [PATCH v1 1/1] USB: serial: Add boundry check for read_urbs array access

[sathyanarayanan kuppuswamy]

On 03/07/2018 12:58 PM, Greg KH wrote:
> On Wed, Mar 07, 2018 at 12:23:56PM -0800, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Sathyanarayanan Kuppuswamy
Linux kernel developer