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

[Greg KH]

On Wed, Mar 07, 2018 at 01:41:51PM -0800, sathyanarayanan kuppuswamy wrote:
> 
> 
> 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),

How is it possible to not have any matching urbs here?  If that ever
happens, we have much worse problems happening in the USB stack :)

thanks,

greg k-h