Re: [PATCH] usb: Avoid use-after-free by flushing endpoints early in usb_set_interface()

[Mathias Nyman <mathias.nyman@linux.intel.com>]

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: <stable@vger.kernel.org>
>> Reported-by: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>   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