From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from netrider.rowland.org ([192.131.102.5]:43969 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1727809AbeHaSyO (ORCPT ); Fri, 31 Aug 2018 14:54:14 -0400 Date: Fri, 31 Aug 2018 10:39:44 -0400 (EDT) From: Alan Stern To: Mathias Nyman cc: linux-usb@vger.kernel.org, , Subject: Re: [PATCH] usb: Avoid use-after-free by flushing endpoints early in usb_set_interface() In-Reply-To: <1535724381-24890-1-git-send-email-mathias.nyman@linux.intel.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: stable-owner@vger.kernel.org List-ID: 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 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: Alan Stern Message-Id: Date: Fri, 31 Aug 2018 10:39:44 -0400 (EDT) To: Mathias Nyman Cc: linux-usb@vger.kernel.org, sudipm.mukherjee@gmail.com, stable@vger.kernel.org List-ID: T24gRnJpLCAzMSBBdWcgMjAxOCwgTWF0aGlhcyBOeW1hbiB3cm90ZToKCj4gVGhlIHN0ZXBzIHRh a2VuIGJ5IHVzYiBjb3JlIHRvIHNldCBhIG5ldyBpbnRlcmZhY2UgaXMgdmVyeSBkaWZmZXJlbnQg ZnJvbQo+IHdoYXQgaXMgZG9uZSBvbiB0aGUgeEhDIGhvc3Qgc2lkZS4KPiAKPiB4SEMgaGFyZHdh cmUgd2lsbCBkbyBldmVyeXRoaW5nIGluIG9uZSBnby4gT25lIGNvbW1hbmQgaXMgdXNlZCB0byBz ZXQgdXAKPiBuZXcgZW5kcG9pbnRzLCBmcmVlIG9sZCBlbmRwb2ludHMsIGNoZWNrIGJhbmR3aWR0 aCwgYW5kIHJ1biB0aGUgbmV3Cj4gZW5kcG9pbnRzLgo+IAo+IEFsbCB0aGlzIGlzIGRvbmUgYnkg eEhDIHdoZW4gdXNiIGNvcmUgYXNrcyB0aGUgaGNkIHRvIGNoZWNrIGZvcgo+IGF2YWlsYWJsZSBi YW5kd2lkdGguIEF0IHRoaXMgcG9pbnQgdXNiIGNvcmUgaGFzIG5vdCB5ZXQgZmx1c2hlZCB0aGUg b2xkCj4gZW5kcG9pbnRzLCB3aGljaCB3aWxsIGNhdXNlIHVzZS1hZnRlci1mcmVlIGlzc3VlcyBp biB4aGNpIGRyaXZlciBhcwo+IHF1ZXVlZCBVUkJzIGFyZSBjYW5jZWxsZWQgb24gYSByZS1hbGxv Y2F0ZWQgZW5kcG9pbnQuCj4gCj4gVG8gcmVzb2x2ZSB0aGlzIGFkZCBhIGNhbGwgdG8gdXNiX2Rp c2FibGVfaW50ZXJmYWNlKCkgd2hpY2ggd2lsbCBmbHVzaAo+IHRoZSBlbmRwb2ludHMgYmVmb3Jl IGNhbGxpbmcgdXNiX2hjZF9hbGxvY19iYW5kd2lkdGgoKQo+IAo+IEFkZGl0aW9uYWwgY2hlY2tz IGluIHhoY2kgZHJpdmVyIHdpbGwgYWxzbyBiZSBpbXBsZW1lbnRlZCB0byBncmFjZWZ1bGx5Cj4g aGFuZGxlIHN0YWxlIFVSQiBjYW5jZWwgb24gZnJlZWQgYW5kIHJlLWFsbG9jYXRlZCBlbmRwb2lu dHMKPiAKPiBDYzogPHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmc+Cj4gUmVwb3J0ZWQtYnk6IFN1ZGlw IE11a2hlcmplZSA8c3VkaXBtLm11a2hlcmplZUBnbWFpbC5jb20+Cj4gU2lnbmVkLW9mZi1ieTog TWF0aGlhcyBOeW1hbiA8bWF0aGlhcy5ueW1hbkBsaW51eC5pbnRlbC5jb20+Cj4gLS0tCj4gIGRy aXZlcnMvdXNiL2NvcmUvbWVzc2FnZS5jIHwgNyArKysrKysrCj4gIDEgZmlsZSBjaGFuZ2VkLCA3 IGluc2VydGlvbnMoKykKPiAKPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy91c2IvY29yZS9tZXNzYWdl LmMgYi9kcml2ZXJzL3VzYi9jb3JlL21lc3NhZ2UuYwo+IGluZGV4IDIyODY3MmYuLjMwNGJlZjIg MTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy91c2IvY29yZS9tZXNzYWdlLmMKPiArKysgYi9kcml2ZXJz L3VzYi9jb3JlL21lc3NhZ2UuYwo+IEBAIC0xMzc3LDYgKzEzNzcsMTMgQEAgaW50IHVzYl9zZXRf aW50ZXJmYWNlKHN0cnVjdCB1c2JfZGV2aWNlICpkZXYsIGludCBpbnRlcmZhY2UsIGludCBhbHRl cm5hdGUpCj4gIAkJcmV0dXJuIC1FSU5WQUw7Cj4gIAl9Cj4gIAo+ICsJLyoKPiArCSAqIHVzYjMg aG9zdHMgY29uZmlndXJlIHRoZSBpbnRlcmZhY2UgaW4gdXNiX2hjZF9hbGxvY19iYW5kd2lkdGgs Cj4gKwkgKiBpbmNsdWRpbmcgZnJlZWluZyBkcm9wcGVkIGVuZHBvaW50IHJpbmcgYnVmZmVycy4K PiArCSAqIE1ha2Ugc3VyZSB0aGUgaW50ZXJmYWNlIGVuZHBvaW50cyBhcmUgZmx1c2hlZCBiZWZv cmUgdGhhdAo+ICsJICovCj4gKwl1c2JfZGlzYWJsZV9pbnRlcmZhY2UoZGV2LCBpZmFjZSwgZmFs c2UpOwo+ICsKPiAgCS8qIE1ha2Ugc3VyZSB3ZSBoYXZlIGVub3VnaCBiYW5kd2lkdGggZm9yIHRo aXMgYWx0ZXJuYXRlIGludGVyZmFjZS4KPiAgCSAqIFJlbW92ZSB0aGUgY3VycmVudCBhbHQgc2V0 dGluZyBhbmQgYWRkIHRoZSBuZXcgYWx0IHNldHRpbmcuCj4gIAkgKi8KClBsZWFzZSBhbHNvIHVw ZGF0ZSB0aGUga2VybmVsZG9jIGZvciB0aGlzIGZ1bmN0aW9uLiAgSXQgc2hvdWxkIG5vdyAKc3Bl Y2lmeSB0aGF0IGlmIHRoZSByZXF1ZXN0IGZhaWxzLCB0aGUgb3JpZ2luYWwgaW50ZXJmYWNlIGFs dHNldHRpbmcgCm1heSBiZSBkaXNhYmxlZC4gIERyaXZlcnMgY2Fubm90IHJlbHkgb24gYW55IHBh cnRpY3VsYXIgYWx0ZXJuYXRlIApzZXR0aW5nIGJlaW5nIGluIGVmZmVjdCBhZnRlciBhIGZhaWx1 cmUuCgpBbGFuIFN0ZXJuCg==