From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v1,1/1] USB: serial: Add boundry check for read_urbs array access From: Greg Kroah-Hartman Message-Id: <20180308140157.GA28178@kroah.com> Date: Thu, 8 Mar 2018 06:01:57 -0800 To: sathyanarayanan kuppuswamy Cc: johan@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org List-ID: T24gV2VkLCBNYXIgMDcsIDIwMTggYXQgMDE6NDE6NTFQTSAtMDgwMCwgc2F0aHlhbmFyYXlhbmFu IGt1cHB1c3dhbXkgd3JvdGU6Cj4gCj4gCj4gT24gMDMvMDcvMjAxOCAxMjo1OCBQTSwgR3JlZyBL SCB3cm90ZToKPiA+IE9uIFdlZCwgTWFyIDA3LCAyMDE4IGF0IDEyOjIzOjU2UE0gLTA4MDAsIHNh dGh5YW5hcmF5YW5hbi5rdXBwdXN3YW15QGxpbnV4LmludGVsLmNvbSB3cm90ZToKPiA+ID4gRnJv bTogS3VwcHVzd2FteSBTYXRoeWFuYXJheWFuYW4gPHNhdGh5YW5hcmF5YW5hbi5rdXBwdXN3YW15 QGxpbnV4LmludGVsLmNvbT4KPiA+ID4gCj4gPiA+IEluIHVzYl9zZXJpYWxfZ2VuZXJpY19zdWJt aXRfcmVhZF91cmIoKSBmdW5jdGlvbiB3ZSBhcmUgYWNjZXNzaW5nIHRoZQo+ID4gPiBwb3J0LT5y ZWFkX3VyYnMgYXJyYXkgd2l0aG91dCBhbnkgYm91bmRyeSBjaGVja3MuIFRoaXMgbWlnaHQgbGVh ZCB0bwo+ID4gPiBrZXJuZWwgcGFuaWMgd2hlbiBpbmRleCB2YWx1ZSBnb2VzIGFib3ZlIGFycmF5 IGxlbmd0aC4KPiA+ID4gCj4gPiA+IE9uZSBwb3NpYmxlIGNhbGwgcGF0aCBmb3IgdGhpcyBpc3N1 ZSBpcywKPiA+ID4gCj4gPiA+IHVzYl9zZXJpYWxfZ2VuZXJpY19yZWFkX2J1bGtfY2FsbGJhY2so KQo+ID4gPiB7Cj4gPiA+ICAgLi4uCj4gPiA+ICAgaWYgKCFwb3J0LT50aHJvdHRsZWQpIHsKPiA+ ID4gCXVzYl9zZXJpYWxfZ2VuZXJpY19zdWJtaXRfcmVhZF91cmIocG9ydCwgaSwgR0ZQX0FUT01J Qyk7Cj4gPiA+ICAgLi4uCj4gPiA+IH0KPiA+IEhvdyBkb2VzIGkgZXZlciBnZXQgdG8gYmUgZ3Jl YXRlciB0aGFuIHRoZSBhcnJheSBzaXplIGhlcmUgaW4gdGhpcwo+ID4gZnVuY3Rpb24/ICBJdCBk aXJlY3RseSBjYW1lIGZyb20gbG9va2luZyBpbiB0aGF0IGFycmF5IGluIHRoZSBmaXJzdAo+ID4g cGxhY2UgOikKPiA+IAo+ID4gU28gSSBkb24ndCBzZWUgd2h5IHlvdXIgY2hlY2sgaXMgbmVlZGVk LCB3aGF0IG90aGVyIGNvZGUgcGF0aCB3b3VsZCBldmVyCj4gPiBjYWxsIHRoaXMgZnVuY3Rpb24g aW4gYSB3YXkgdGhhdCB0aGUgYm91bmRzIGNoZWNrIHdvdWxkIGJlIG5lZWRlZD8KPiB2b2lkIHVz Yl9zZXJpYWxfZ2VuZXJpY19yZWFkX2J1bGtfY2FsbGJhY2soc3RydWN0IHVyYiAqdXJiKQo+IAo+ IDM4NcKgwqDCoMKgwqDCoMKgwqAgZm9yIChpID0gMDsgaSA8IEFSUkFZX1NJWkUocG9ydC0+cmVh ZF91cmJzKTsgKytpKSB7Cj4gMzg2wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgaWYg KHVyYiA9PSBwb3J0LT5yZWFkX3VyYnNbaV0pCj4gMzg3wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIGJyZWFrOwo+IDM4OMKgwqDCoMKgwqDCoMKgwqAgfQo+ IAo+IEluIGhlcmUsIGFmdGVyIHRoaXMgZm9yIGxvb3AgaXMgZG9uZSAod2l0aG91dCBhbnkgbWF0 Y2hpbmcgdXJiKSwKCkhvdyBpcyBpdCBwb3NzaWJsZSB0byBub3QgaGF2ZSBhbnkgbWF0Y2hpbmcg dXJicyBoZXJlPyAgSWYgdGhhdCBldmVyCmhhcHBlbnMsIHdlIGhhdmUgbXVjaCB3b3JzZSBwcm9i bGVtcyBoYXBwZW5pbmcgaW4gdGhlIFVTQiBzdGFjayA6KQoKdGhhbmtzLAoKZ3JlZyBrLWgKLS0t ClRvIHVuc3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmli ZSBsaW51eC11c2IiIGluCnRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5r ZXJuZWwub3JnCk1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcv bWFqb3Jkb21vLWluZm8uaHRtbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Thu, 8 Mar 2018 06:01:57 -0800 From: Greg KH To: sathyanarayanan kuppuswamy Cc: johan@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 1/1] USB: serial: Add boundry check for read_urbs array access Message-ID: <20180308140157.GA28178@kroah.com> References: <20180307205840.GA6242@kroah.com> <0055f93b-8497-5dfc-4233-9cc72bf690fc@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0055f93b-8497-5dfc-4233-9cc72bf690fc@linux.intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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@linux.intel.com wrote: > > > From: Kuppuswamy Sathyanarayanan > > > > > > 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