From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com ([134.134.136.65]:32406 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726023AbeICQyM (ORCPT ); Mon, 3 Sep 2018 12:54:12 -0400 Subject: Re: [PATCH] usb: Avoid use-after-free by flushing endpoints early in usb_set_interface() To: Alan Stern Cc: linux-usb@vger.kernel.org, sudipm.mukherjee@gmail.com, stable@vger.kernel.org References: From: Mathias Nyman Message-ID: <4cf27ab2-ecd4-2288-749f-77e35bec344e@linux.intel.com> Date: Mon, 3 Sep 2018 15:36:25 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: On 31.08.2018 17:39, Alan Stern wrote: > On Fri, 31 Aug 2018, Mathias Nyman wrote: > >> The steps taken by usb core to set a new interface is very different from >> what is done on the xHC host side. >> >> xHC hardware will do everything in one go. One command is used to set up >> new endpoints, free old endpoints, check bandwidth, and run the new >> endpoints. >> >> All this is done by xHC when usb core asks the hcd to check for >> available bandwidth. At this point usb core has not yet flushed the old >> endpoints, which will cause use-after-free issues in xhci driver as >> queued URBs are cancelled on a re-allocated endpoint. >> >> To resolve this add a call to usb_disable_interface() which will flush >> the endpoints before calling usb_hcd_alloc_bandwidth() >> >> Additional checks in xhci driver will also be implemented to gracefully >> handle stale URB cancel on freed and re-allocated endpoints >> >> Cc: >> Reported-by: Sudip Mukherjee >> Signed-off-by: Mathias Nyman >> --- >> drivers/usb/core/message.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c >> index 228672f..304bef2 100644 >> --- a/drivers/usb/core/message.c >> +++ b/drivers/usb/core/message.c >> @@ -1377,6 +1377,13 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) >> return -EINVAL; >> } >> >> + /* >> + * usb3 hosts configure the interface in usb_hcd_alloc_bandwidth, >> + * including freeing dropped endpoint ring buffers. >> + * Make sure the interface endpoints are flushed before that >> + */ >> + usb_disable_interface(dev, iface, false); >> + >> /* Make sure we have enough bandwidth for this alternate interface. >> * Remove the current alt setting and add the new alt setting. >> */ > > Please also update the kerneldoc for this function. It should now > specify that if the request fails, the original interface altsetting > may be disabled. Drivers cannot rely on any particular alternate > setting being in effect after a failure. > > Alan Stern > Sure, thanks, will do -Mathias 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: usb: Avoid use-after-free by flushing endpoints early in usb_set_interface() From: Mathias Nyman Message-Id: <4cf27ab2-ecd4-2288-749f-77e35bec344e@linux.intel.com> Date: Mon, 3 Sep 2018 15:36:25 +0300 To: Alan Stern Cc: linux-usb@vger.kernel.org, sudipm.mukherjee@gmail.com, stable@vger.kernel.org List-ID: T24gMzEuMDguMjAxOCAxNzozOSwgQWxhbiBTdGVybiB3cm90ZToKPiBPbiBGcmksIDMxIEF1ZyAy MDE4LCBNYXRoaWFzIE55bWFuIHdyb3RlOgo+IAo+PiBUaGUgc3RlcHMgdGFrZW4gYnkgdXNiIGNv cmUgdG8gc2V0IGEgbmV3IGludGVyZmFjZSBpcyB2ZXJ5IGRpZmZlcmVudCBmcm9tCj4+IHdoYXQg aXMgZG9uZSBvbiB0aGUgeEhDIGhvc3Qgc2lkZS4KPj4KPj4geEhDIGhhcmR3YXJlIHdpbGwgZG8g ZXZlcnl0aGluZyBpbiBvbmUgZ28uIE9uZSBjb21tYW5kIGlzIHVzZWQgdG8gc2V0IHVwCj4+IG5l dyBlbmRwb2ludHMsIGZyZWUgb2xkIGVuZHBvaW50cywgY2hlY2sgYmFuZHdpZHRoLCBhbmQgcnVu IHRoZSBuZXcKPj4gZW5kcG9pbnRzLgo+Pgo+PiBBbGwgdGhpcyBpcyBkb25lIGJ5IHhIQyB3aGVu IHVzYiBjb3JlIGFza3MgdGhlIGhjZCB0byBjaGVjayBmb3IKPj4gYXZhaWxhYmxlIGJhbmR3aWR0 aC4gQXQgdGhpcyBwb2ludCB1c2IgY29yZSBoYXMgbm90IHlldCBmbHVzaGVkIHRoZSBvbGQKPj4g ZW5kcG9pbnRzLCB3aGljaCB3aWxsIGNhdXNlIHVzZS1hZnRlci1mcmVlIGlzc3VlcyBpbiB4aGNp IGRyaXZlciBhcwo+PiBxdWV1ZWQgVVJCcyBhcmUgY2FuY2VsbGVkIG9uIGEgcmUtYWxsb2NhdGVk IGVuZHBvaW50Lgo+Pgo+PiBUbyByZXNvbHZlIHRoaXMgYWRkIGEgY2FsbCB0byB1c2JfZGlzYWJs ZV9pbnRlcmZhY2UoKSB3aGljaCB3aWxsIGZsdXNoCj4+IHRoZSBlbmRwb2ludHMgYmVmb3JlIGNh bGxpbmcgdXNiX2hjZF9hbGxvY19iYW5kd2lkdGgoKQo+Pgo+PiBBZGRpdGlvbmFsIGNoZWNrcyBp biB4aGNpIGRyaXZlciB3aWxsIGFsc28gYmUgaW1wbGVtZW50ZWQgdG8gZ3JhY2VmdWxseQo+PiBo YW5kbGUgc3RhbGUgVVJCIGNhbmNlbCBvbiBmcmVlZCBhbmQgcmUtYWxsb2NhdGVkIGVuZHBvaW50 cwo+Pgo+PiBDYzogPHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmc+Cj4+IFJlcG9ydGVkLWJ5OiBTdWRp cCBNdWtoZXJqZWUgPHN1ZGlwbS5tdWtoZXJqZWVAZ21haWwuY29tPgo+PiBTaWduZWQtb2ZmLWJ5 OiBNYXRoaWFzIE55bWFuIDxtYXRoaWFzLm55bWFuQGxpbnV4LmludGVsLmNvbT4KPj4gLS0tCj4+ ICAgZHJpdmVycy91c2IvY29yZS9tZXNzYWdlLmMgfCA3ICsrKysrKysKPj4gICAxIGZpbGUgY2hh bmdlZCwgNyBpbnNlcnRpb25zKCspCj4+Cj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL3VzYi9jb3Jl L21lc3NhZ2UuYyBiL2RyaXZlcnMvdXNiL2NvcmUvbWVzc2FnZS5jCj4+IGluZGV4IDIyODY3MmYu LjMwNGJlZjIgMTAwNjQ0Cj4+IC0tLSBhL2RyaXZlcnMvdXNiL2NvcmUvbWVzc2FnZS5jCj4+ICsr KyBiL2RyaXZlcnMvdXNiL2NvcmUvbWVzc2FnZS5jCj4+IEBAIC0xMzc3LDYgKzEzNzcsMTMgQEAg aW50IHVzYl9zZXRfaW50ZXJmYWNlKHN0cnVjdCB1c2JfZGV2aWNlICpkZXYsIGludCBpbnRlcmZh Y2UsIGludCBhbHRlcm5hdGUpCj4+ICAgCQlyZXR1cm4gLUVJTlZBTDsKPj4gICAJfQo+PiAgIAo+ PiArCS8qCj4+ICsJICogdXNiMyBob3N0cyBjb25maWd1cmUgdGhlIGludGVyZmFjZSBpbiB1c2Jf aGNkX2FsbG9jX2JhbmR3aWR0aCwKPj4gKwkgKiBpbmNsdWRpbmcgZnJlZWluZyBkcm9wcGVkIGVu ZHBvaW50IHJpbmcgYnVmZmVycy4KPj4gKwkgKiBNYWtlIHN1cmUgdGhlIGludGVyZmFjZSBlbmRw b2ludHMgYXJlIGZsdXNoZWQgYmVmb3JlIHRoYXQKPj4gKwkgKi8KPj4gKwl1c2JfZGlzYWJsZV9p bnRlcmZhY2UoZGV2LCBpZmFjZSwgZmFsc2UpOwo+PiArCj4+ICAgCS8qIE1ha2Ugc3VyZSB3ZSBo YXZlIGVub3VnaCBiYW5kd2lkdGggZm9yIHRoaXMgYWx0ZXJuYXRlIGludGVyZmFjZS4KPj4gICAJ ICogUmVtb3ZlIHRoZSBjdXJyZW50IGFsdCBzZXR0aW5nIGFuZCBhZGQgdGhlIG5ldyBhbHQgc2V0 dGluZy4KPj4gICAJICovCj4gCj4gUGxlYXNlIGFsc28gdXBkYXRlIHRoZSBrZXJuZWxkb2MgZm9y IHRoaXMgZnVuY3Rpb24uICBJdCBzaG91bGQgbm93Cj4gc3BlY2lmeSB0aGF0IGlmIHRoZSByZXF1 ZXN0IGZhaWxzLCB0aGUgb3JpZ2luYWwgaW50ZXJmYWNlIGFsdHNldHRpbmcKPiBtYXkgYmUgZGlz YWJsZWQuICBEcml2ZXJzIGNhbm5vdCByZWx5IG9uIGFueSBwYXJ0aWN1bGFyIGFsdGVybmF0ZQo+ IHNldHRpbmcgYmVpbmcgaW4gZWZmZWN0IGFmdGVyIGEgZmFpbHVyZS4KPiAKPiBBbGFuIFN0ZXJu Cj4gCgpTdXJlLCB0aGFua3MsIHdpbGwgZG8KCi1NYXRoaWFzCg==