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]

Date:   Thu, 8 Mar 2018 06:01:57 -0800
From:   Greg KH <gregkh@...uxfoundation.org>
To:     sathyanarayanan kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>
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 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@...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),

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