linux-kernel - Re: [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during bus reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d77bbea0-04eb-9426-5b16-50e408b2e787@codeaurora.org>
Date:   Fri, 19 Mar 2021 22:30:36 -0700
From:   Wesley Cheng <wcheng@...eaurora.org>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH 2/2] usb: dwc3: gadget: Ignore EP queue requests during
 bus reset

Hi Thinh,


On 3/19/2021 7:01 PM, Thinh Nguyen wrote:
> Wesley Cheng wrote:
>>
>>
>> On 3/19/2021 5:40 PM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> Wesley Cheng wrote:
>>>> The current dwc3_gadget_reset_interrupt() will stop any active
>>>> transfers, but only addresses blocking of EP queuing for while we are
>>>> coming from a disconnected scenario, i.e. after receiving the disconnect
>>>> event.  If the host decides to issue a bus reset on the device, the
>>>> connected parameter will still be set to true, allowing for EP queuing
>>>> to continue while we are disabling the functions.  To avoid this, set the
>>>> connected flag to false until the stop active transfers is complete.
>>>>
>>>> Signed-off-by: Wesley Cheng <wcheng@...eaurora.org>
>>>> ---
>>>>  drivers/usb/dwc3/gadget.c | 9 +++++++++
>>>>  1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index 6e14fdc..d5ed0f69 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -3327,6 +3327,15 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>>>>  	u32			reg;
>>>>  
>>>>  	/*
>>>> +	 * Ideally, dwc3_reset_gadget() would trigger the function
>>>> +	 * drivers to stop any active transfers through ep disable.
>>>> +	 * However, for functions which defer ep disable, such as mass
>>>> +	 * storage, we will need to rely on the call to stop active
>>>> +	 * transfers here, and avoid allowing of request queuing.
>>>> +	 */
>>>> +	dwc->connected = false;
>>>> +
>>>> +	/*
>>>>  	 * WORKAROUND: DWC3 revisions <1.88a have an issue which
>>>>  	 * would cause a missing Disconnect Event if there's a
>>>>  	 * pending Setup Packet in the FIFO.
>>>>
>>>
>>> This doesn't look right. Did you have rebase issue with your local
>>> change again?
>>>
>>> BR,
>>> Thinh
>>>
>> Hi Thinh,
>>
>> This was rebased on Greg's usb-linus branch, which has commit
>> f09ddcfcb8c5 ("usb: dwc3: gadget: Prevent EP queuing while stopping
>> transfers") merged.
> 
> Ah I see.
> 
>>
>> commit f09ddcfcb8c5  moved the dwc->connected = true to after we have
>> finished stop active transfers.  However, this change will also ensure
>> that the connected flag is set to false to ensure that when we call stop
>> active transfers, nothing can prepare TRBs.  (previous commit only
>> addresses the case where we get the reset interrupt when coming from a
>> disconnected state)
>>
> 
> That still doesn't address this issue.
> 
> Because:
> 1) We're still protected by the spin_lock_irq*(), so this change doesn't
> make any difference while handling an event.

Thank you for the feedback.  So it is true that we lock dwc->lock while
handling EP/device events, but what these changes are trying to address
is that during dwc3_stop_active_transfers() we will eventually call
dwc3_gadget_giveback() to call the complete() functions registered by
the function driver.  Before we call the complete() callbacks, we unlock
dwc->lock, so we are no longer protected, and if there was a pending ep
queue from a function driver, that would allow it to acquire the lock
and continue preparing the TRBs.

> 2) We don't enable the interrupt for END_TRANSFER command completion
> when doing dwc3_stop_active_transfers(), the
> DWC3_EP_END_TRANSFER_PENDING flag will not be set to prevent preparing
> new requests.
> 
Agreed.  That is the reason for adding the check to dwc->connected in
__dwc3_gadget_ep_queue()

if (!dep->endpoint.desc || !dwc->pullups_connected || !dwc->connected) {
 		dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
 				dep->name);
 		return -ESHUTDOWN;

> We should do dwc->connected = true when we handle connection_done
> interrupt instead. The END_TRANSFER command should complete before this.
> 
So how this change will address the issue is:

1.  IRQ handler will acquire dwc->lock
2.  dwc3_gadget_reset_handler() sets dwc->connected = false
3.  Call to dwc3_stop_active_transfers()
	---> dwc3_gadget_giveback() releases dwc->lock
4.  If there was a pending ep queue (waiting for dwc->lock) it can
continue here
5.  __dwc3_gadget_ep_queue() exits early due to dwc->connected = false
6.  dwc3_gadget_giveback() re-acquires dwc->lock and continues

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project