linux-kernel - Re: [RFC PATCH v2 1/3] usb: dwc3: Flush pending SETUP data during stop active xfers

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: <469213d0-526a-e26e-88c2-b34b2aa8dfe7@codeaurora.org>
Date:   Tue, 22 Mar 2022 22:47:58 -0700
From:   Wesley Cheng <wcheng@...eaurora.org>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Wesley Cheng <quic_wcheng@...cinc.com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Cc:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>
Subject: Re: [RFC PATCH v2 1/3] usb: dwc3: Flush pending SETUP data during
 stop active xfers

Hi Thinh,

On 3/22/2022 6:31 PM, Thinh Nguyen wrote:
> Hi Wesley,
> 
> Wesley Cheng wrote:
>> Hi Thinh,
>>
>> On 2/15/2022 4:08 PM, Wesley Cheng wrote:
>>> While running the pullup disable sequence, if there are pending SETUP
>>> transfers stored in the controller, then the ENDTRANSFER commands on non
>>> control eps will fail w/ ETIMEDOUT.  As a suggestion from SNPS, in order
>>> to drain potentially cached SETUP packets, SW needs to issue a
>>> STARTTRANSFER command.  After issuing the STARTTRANSFER, and retrying the
>>> ENDTRANSFER, the command should succeed.  Else, if the endpoints are not
>>> properly stopped, the controller halt sequence will fail as well.
>>>
>>> One limitation is that the current logic will drop the SETUP data
>>> being received (ie dropping the SETUP packet), however, it should be
>>> acceptable in the pullup disable case, as the device is eventually
>>> going to disconnect from the host.
>>>
>>> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
>>> ---
>> Just wondering if you had any inputs for this particular change?  I
>> think on the dequeue path discussion you had some concerns with parts of
>> this change?  I think the difficult part for the pullup disable path is
>> that we have this API running w/ interrupts disabled, so the EP0 state
>> won't be able to advance compared to the dequeue case.
> 
> This doesn't sound right. The pullup disable (or device initiated
> disconnect) should wait for the EP0 state to be EP0_SETUP_PHASE before
> proceeding, which it does.
> 
That is correct, but even though that check is passed, it doesn't
prevent the host from sending another SETUP token between the pending
setup packet check and run/stop clear.

>>
>> Ideally, if there is a way to ensure that we avoid reading further setup
>> packets, that would be nice, but from our discussions with Synopsys,
>> this was not possible. (controller is always armed and ready to ACK
>> setup tokens)
>>
>> I did evaluate keeping IRQs enabled and periodically releasing/attaining
>> the lock between the stop active xfer calls, but that opened another can
>> of worms.  If you think this is the approach we should take, I can take
>> a look at this implementation further.
>>
> 
> This patch doesn't look right to me. The change I suggested before
> should address this (I believe Greg already picked it up). What other
> problem do you see, I'm not clear what's the problem here. One potential
> problem that I can see is that currently dwc3 driver doesn't wait for
> active endpoints to complete/end before clearing the run_stop bit on
> device initiated disconnect, but I'm not sure if that's what you're seeing.
> 
> Please help clarify further. If there's trace points log, that'd help.
> 
Main difference between the issue Greg recently pulled in and this one
is that this is on the pullup disable patch (no dequeue involved).  In
this situation we will also stop active transfers, so that the
controller can halt properly.

I attached a few files, and will give a summary of them below:
1.  pullup_disable_timeout.txt - ftrace of an instance of when we see
several endxfer timeouts.  Refer to line#2016.

2.  lecroy+ftrace_snip.png - picture showing a matching bus sniffer log
and a ftrace collected (difference instance to #1)

#2 will show that we completed a SETUP transfer before entering into
dwc3_gadget_stop_active_transfers().  In here, we then see DWC ACK
another SETUP token, which leads to endxfer timeouts on all subsequent
endpoints.

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
View attachment "pullup_disable_timeout.txt" of type "text/plain" (300721 bytes)

Download attachment "lecroy+ftrace_snip.png" of type "image/png" (142903 bytes)