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: <20180308234306.GA22931@kroah.com> Date: Thu, 8 Mar 2018 15:43:06 -0800 To: sathyanarayanan kuppuswamy Cc: Oliver Neukum , johan@kernel.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org List-ID: T24gVGh1LCBNYXIgMDgsIDIwMTggYXQgMDM6Mjk6NDhQTSAtMDgwMCwgc2F0aHlhbmFyYXlhbmFu IGt1cHB1c3dhbXkgd3JvdGU6Cj4gCj4gCj4gT24gMDMvMDgvMjAxOCAxMjo1NCBBTSwgT2xpdmVy IE5ldWt1bSB3cm90ZToKPiA+IEFtIE1pdHR3b2NoLCBkZW4gMDcuMDMuMjAxOCwgMTM6NDEgLTA4 MDAgc2NocmllYiBzYXRoeWFuYXJheWFuYW4KPiA+IGt1cHB1c3dhbXkgICAgICAgOgo+ID4gPiBP biAwMy8wNy8yMDE4IDEyOjU4IFBNLCBHcmVnIEtIIHdyb3RlOgo+ID4gPiA+IFNvIEkgZG9uJ3Qg c2VlIHdoeSB5b3VyIGNoZWNrIGlzIG5lZWRlZCwgd2hhdCBvdGhlciBjb2RlIHBhdGggd291bGQg ZXZlcgo+ID4gPiA+IGNhbGwgdGhpcyBmdW5jdGlvbiBpbiBhIHdheSB0aGF0IHRoZSBib3VuZHMg Y2hlY2sgd291bGQgYmUgbmVlZGVkPwo+ID4gPiB2b2lkIHVzYl9zZXJpYWxfZ2VuZXJpY19yZWFk X2J1bGtfY2FsbGJhY2soc3RydWN0IHVyYiAqdXJiKQo+ID4gPiAKPiA+ID4gMzg1wqDCoMKgwqDC oMKgwqDCoCBmb3IgKGkgPSAwOyBpIDwgQVJSQVlfU0laRShwb3J0LT5yZWFkX3VyYnMpOyArK2kp IHsKPiA+ID4gMzg2wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgaWYgKHVyYiA9PSBw b3J0LT5yZWFkX3VyYnNbaV0pCj4gPiA+IDM4N8KgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoCBicmVhazsKPiA+ID4gMzg4wqDCoMKgwqDCoMKgwqDCoCB9Cj4g PiA+IAo+ID4gPiBJbiBoZXJlLCBhZnRlciB0aGlzIGZvciBsb29wIGlzIGRvbmUgKHdpdGhvdXQg YW55IG1hdGNoaW5nIHVyYiksIGkgdmFsdWUKPiA+ID4gd2lsbCBiZSBlcXVhbCB0byBBUlJBWV9T SVpFKHBvcnQtPnJlYWRfdXJicykuIFNvIHRoZXJlIGlzIGEgcG9zc2liaWxpdHkKPiA+ID4gb2Yg dXNiX3NlcmlhbF9nZW5lcmljX3N1Ym1pdF9yZWFkX3VyYigpIGdldHRpbmcgY2FsbGVkIHdpdGgg dGhpcyBpbnZhbGlkCj4gPiA+IGluZGV4Lgo+ID4gSWYgdGhpcyBoYXBwZW5zIHRoZSBmdW5jdGlv biB3YXMgY2FsbGVkIGZvciBhIHN0cmF5IFVSQi4KPiA+IFlvdXIgY2hlY2sgY29tZXMgdG8gbGF0 ZS4gV2UgaGF2ZSBjYWxsZWQgc2V0X2JpdCB3aXRoIGFuIGludmFsaWQgaW5kZXgKPiA+IGFuZCBv dGhlciBzaGl0Lgo+ID4gV2UgZGVmaW5pdGVseSBkbyBub3QganVzdCB3YW50IHRvIHJldHVybiBh biBlcnJvciBpbiB0aGF0IGNhc2UuCj4gSW4gdGhhdCBjYXNlIGRvIHlvdSB0aGluayB3ZSBzaG91 bGQgdXNlIHNvbWUgV0FSTl9PTigpIGZvciBpbnZhbGlkIGluZGV4IGluCj4gdXNiX3NlcmlhbF9n ZW5lcmljX3JlYWRfYnVsa19jYWxsYmFjaygpPwoKTm8sIGFnYWluLCBob3cgY291bGQgdGhhdCBl dmVyIGhhcHBlbj8KCkRvbid0IGFkZCBwb2ludGxlc3MgZXJyb3IgY2hlY2tpbmcgZm9yIHRoaW5n cyB0aGF0IGFyZSBpbXBvc3NpYmxlIHRvCmV2ZXIgaGl0IDopCgp0aGFua3MsCgpncmVnIGstaAot LS0KVG8gdW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2Ny aWJlIGxpbnV4LXVzYiIgaW4KdGhlIGJvZHkgb2YgYSBtZXNzYWdlIHRvIG1ham9yZG9tb0B2Z2Vy Lmtlcm5lbC5vcmcKTW9yZSBtYWpvcmRvbW8gaW5mbyBhdCAgaHR0cDovL3ZnZXIua2VybmVsLm9y Zy9tYWpvcmRvbW8taW5mby5odG1sCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Thu, 8 Mar 2018 15:43:06 -0800 From: Greg KH To: sathyanarayanan kuppuswamy Cc: Oliver Neukum , johan@kernel.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Subject: Re: [PATCH v1 1/1] USB: serial: Add boundry check for read_urbs array access Message-ID: <20180308234306.GA22931@kroah.com> References: <20180307205840.GA6242@kroah.com> <0055f93b-8497-5dfc-4233-9cc72bf690fc@linux.intel.com> <1520499297.2983.3.camel@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 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 k-h