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: sathyanarayanan.kuppuswamy@linux.intel.com Message-Id: Date: Thu, 8 Mar 2018 16:34:44 -0800 To: Greg KH Cc: Oliver Neukum , johan@kernel.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org List-ID: T24gMDMvMDgvMjAxOCAwMzo0MyBQTSwgR3JlZyBLSCB3cm90ZToKPiBPbiBUaHUsIE1hciAwOCwg MjAxOCBhdCAwMzoyOTo0OFBNIC0wODAwLCBzYXRoeWFuYXJheWFuYW4ga3VwcHVzd2FteSB3cm90 ZToKPj4KPj4gT24gMDMvMDgvMjAxOCAxMjo1NCBBTSwgT2xpdmVyIE5ldWt1bSB3cm90ZToKPj4+ IEFtIE1pdHR3b2NoLCBkZW4gMDcuMDMuMjAxOCwgMTM6NDEgLTA4MDAgc2NocmllYiBzYXRoeWFu YXJheWFuYW4KPj4+IGt1cHB1c3dhbXkgICAgICAgOgo+Pj4+IE9uIDAzLzA3LzIwMTggMTI6NTgg UE0sIEdyZWcgS0ggd3JvdGU6Cj4+Pj4+IFNvIEkgZG9uJ3Qgc2VlIHdoeSB5b3VyIGNoZWNrIGlz IG5lZWRlZCwgd2hhdCBvdGhlciBjb2RlIHBhdGggd291bGQgZXZlcgo+Pj4+PiBjYWxsIHRoaXMg ZnVuY3Rpb24gaW4gYSB3YXkgdGhhdCB0aGUgYm91bmRzIGNoZWNrIHdvdWxkIGJlIG5lZWRlZD8K Pj4+PiB2b2lkIHVzYl9zZXJpYWxfZ2VuZXJpY19yZWFkX2J1bGtfY2FsbGJhY2soc3RydWN0IHVy YiAqdXJiKQo+Pj4+Cj4+Pj4gMzg1wqDCoMKgwqDCoMKgwqDCoCBmb3IgKGkgPSAwOyBpIDwgQVJS QVlfU0laRShwb3J0LT5yZWFkX3VyYnMpOyArK2kpIHsKPj4+PiAzODbCoMKgwqDCoMKgwqDCoMKg wqDCoMKgwqDCoMKgwqDCoCBpZiAodXJiID09IHBvcnQtPnJlYWRfdXJic1tpXSkKPj4+PiAzODfC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgYnJlYWs7Cj4+ Pj4gMzg4wqDCoMKgwqDCoMKgwqDCoCB9Cj4+Pj4KPj4+PiBJbiBoZXJlLCBhZnRlciB0aGlzIGZv ciBsb29wIGlzIGRvbmUgKHdpdGhvdXQgYW55IG1hdGNoaW5nIHVyYiksIGkgdmFsdWUKPj4+PiB3 aWxsIGJlIGVxdWFsIHRvIEFSUkFZX1NJWkUocG9ydC0+cmVhZF91cmJzKS4gU28gdGhlcmUgaXMg YSBwb3NzaWJpbGl0eQo+Pj4+IG9mIHVzYl9zZXJpYWxfZ2VuZXJpY19zdWJtaXRfcmVhZF91cmIo KSBnZXR0aW5nIGNhbGxlZCB3aXRoIHRoaXMgaW52YWxpZAo+Pj4+IGluZGV4Lgo+Pj4gSWYgdGhp cyBoYXBwZW5zIHRoZSBmdW5jdGlvbiB3YXMgY2FsbGVkIGZvciBhIHN0cmF5IFVSQi4KPj4+IFlv dXIgY2hlY2sgY29tZXMgdG8gbGF0ZS4gV2UgaGF2ZSBjYWxsZWQgc2V0X2JpdCB3aXRoIGFuIGlu dmFsaWQgaW5kZXgKPj4+IGFuZCBvdGhlciBzaGl0Lgo+Pj4gV2UgZGVmaW5pdGVseSBkbyBub3Qg anVzdCB3YW50IHRvIHJldHVybiBhbiBlcnJvciBpbiB0aGF0IGNhc2UuCj4+IEluIHRoYXQgY2Fz ZSBkbyB5b3UgdGhpbmsgd2Ugc2hvdWxkIHVzZSBzb21lIFdBUk5fT04oKSBmb3IgaW52YWxpZCBp bmRleCBpbgo+PiB1c2Jfc2VyaWFsX2dlbmVyaWNfcmVhZF9idWxrX2NhbGxiYWNrKCk/Cj4gTm8s IGFnYWluLCBob3cgY291bGQgdGhhdCBldmVyIGhhcHBlbj8KPgo+IERvbid0IGFkZCBwb2ludGxl c3MgZXJyb3IgY2hlY2tpbmcgZm9yIHRoaW5ncyB0aGF0IGFyZSBpbXBvc3NpYmxlIHRvCj4gZXZl ciBoaXQgOikKVGhhbmtzIEdyZWcuCj4KPiB0aGFua3MsCj4KPiBncmVnIGstaAo+IC0tCj4gVG8g dW5zdWJzY3JpYmUgZnJvbSB0aGlzIGxpc3Q6IHNlbmQgdGhlIGxpbmUgInVuc3Vic2NyaWJlIGxp bnV4LXVzYiIgaW4KPiB0aGUgYm9keSBvZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2Vy bmVsLm9yZwo+IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcv bWFqb3Jkb21vLWluZm8uaHRtbAo+Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AG47ELtc9WBmIPQmG/Yrn4IB7p8WouyqeMKy0Fp1X0bEBOB23dHpZQH3Ry9LI+GeCxlvWl1D65Vj ARC-Seal: i=1; a=rsa-sha256; t=1520555699; cv=none; d=google.com; s=arc-20160816; b=KtyNv291LRXFG/Pixr9EjT4YJhcsjGMFDZqo/g0tqDrS3J+1sucC4UwXMQh3D1eWpX epcoak51ztGiCLvMeEoz3R+C6Y9qWjs1pz9cAglfd9naCiNr1X/VEgZv3vNNIJoiHccc yqjBcZNHqVokbKkgn3AO49m+rgorMBLH4OllMGuTl2lRlv3iblIjNn25qXVuHIvRX/Q+ 4ne1p5HEwh3EnUVqIX8MS9ZdBvN3pAACrUD8BOwjG0/5waqIf089M241gMRSrKqPL3Np h1W5/Vxs4LgENEXvuzNoJjmJKydlI/9zLUJqrA4RNJ7Rip+4z0UVt7kngiODFpB89i8U G3vA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-language:content-transfer-encoding:in-reply-to:mime-version :user-agent:date:message-id:organization:from:references:cc:to :subject:reply-to:arc-authentication-results; bh=23pHynXASDaoQHhWVaXGOyR4vJbMKcYujhbgpRpIlZk=; b=zkzqxDRfdGbvmIh3PtjCZtusyxeEEnFgwR2oyFTtVYxAqu1yfGgzPhWhsDl1QxlsI1 84dphUlzNj+52qFwiEQEaNeegSFgFgERjuYwHsor12FM6b2BLTyhuqIbKH7yG2MkqKds AEWf2/aTqFrCHjIH5DWFcnMJiMw53SMYHhGywDHDf3+QUmPqWMeNu5ifmLK1xQrQLCfn 2ZuadQjqoSfcy/G4M+SykmIM7OlAI3qPwAqmBJzg634DuadBVS861BqO7eCMfbB77Wnt OgC6bWWAhg5QYikRvDIESS9mNEx+2Gbz6ng2NOyHUmfznvwdN4qdecp2IFNahQ/FQQLj 63WQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of sathyanarayanan.kuppuswamy@linux.intel.com designates 134.134.136.31 as permitted sender) smtp.mailfrom=sathyanarayanan.kuppuswamy@linux.intel.com Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of sathyanarayanan.kuppuswamy@linux.intel.com designates 134.134.136.31 as permitted sender) smtp.mailfrom=sathyanarayanan.kuppuswamy@linux.intel.com X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,443,1515484800"; d="scan'208";a="37187649" Reply-To: sathyanarayanan.kuppuswamy@linux.intel.com Subject: Re: [PATCH v1 1/1] USB: serial: Add boundry check for read_urbs array access To: Greg KH Cc: Oliver Neukum , johan@kernel.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org References: <20180307205840.GA6242@kroah.com> <0055f93b-8497-5dfc-4233-9cc72bf690fc@linux.intel.com> <1520499297.2983.3.camel@suse.com> <20180308234306.GA22931@kroah.com> From: sathyanarayanan kuppuswamy Organization: Intel Message-ID: Date: Thu, 8 Mar 2018 16:34:44 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180308234306.GA22931@kroah.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1594311837404941668?= X-GMAIL-MSGID: =?utf-8?q?1594418212316121088?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: On 03/08/2018 03:43 PM, Greg KH wrote: > 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. > > thanks, > > greg k-h > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Sathyanarayanan Kuppuswamy Linux kernel developer