linux-kernel - RE: [PATCH] usb: cdns3: remove fetched trb from cache before dequeuing

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]

Date:   Thu, 22 Dec 2022 09:23:14 +0000
From:   Pawel Laszczak <pawell@...ence.com>
To:     Peter Chen <hzpeterchen@...il.com>
CC:     "peter.chen@...nel.org" <peter.chen@...nel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "felipe.balbi@...ux.intel.com" <felipe.balbi@...ux.intel.com>,
        "rogerq@...nel.org" <rogerq@...nel.org>,
        "a-govindraju@...com" <a-govindraju@...com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: RE: [PATCH] usb: cdns3: remove fetched trb from cache before
 dequeuing

Hi Peter,

What about this patch?
Can you push it?

Regards,
Pawel

>>On Tue, Nov 15, 2022 at 6:01 PM Pawel Laszczak <pawell@...ence.com>
>>wrote:
>>>
>>> After doorbell DMA fetches the TRB. If during dequeuing request
>>> driver changes NORMAL TRB to LINK TRB but doesn't delete it from
>>> controller cache then controller will handle cached TRB and packet can be
>lost.
>>>
>>> The example scenario for this issue looks like:
>>> 1. queue request - set doorbell
>>> 2. dequeue request
>>> 3. send OUT data packet from host
>>> 4. Device will accept this packet which is unexpected 5. queue new
>>> request - set doorbell 6. Device lost the expected packet.
>>>
>>> By setting DFLUSH controller clears DRDY bit and stop DMA transfer.
>>>
>>> Fixes: 7733f6c32e36 ("usb: cdns3: Add Cadence USB3 DRD Driver")
>>> cc: <stable@...r.kernel.org>
>>> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
>>> ---
>>>  drivers/usb/cdns3/cdns3-gadget.c | 12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/usb/cdns3/cdns3-gadget.c
>>> b/drivers/usb/cdns3/cdns3-gadget.c
>>> index 5adcb349718c..ccfaebca6faa 100644
>>> --- a/drivers/usb/cdns3/cdns3-gadget.c
>>> +++ b/drivers/usb/cdns3/cdns3-gadget.c
>>> @@ -2614,6 +2614,7 @@ int cdns3_gadget_ep_dequeue(struct usb_ep *ep,
>>>         u8 req_on_hw_ring = 0;
>>>         unsigned long flags;
>>>         int ret = 0;
>>> +       int val;
>>>
>>>         if (!ep || !request || !ep->desc)
>>>                 return -EINVAL;
>>> @@ -2649,6 +2650,13 @@ int cdns3_gadget_ep_dequeue(struct usb_ep
>>*ep,
>>>
>>>         /* Update ring only if removed request is on pending_req_list list */
>>>         if (req_on_hw_ring && link_trb) {
>>> +               /* Stop DMA */
>>> +               writel(EP_CMD_DFLUSH, &priv_dev->regs->ep_cmd);
>>> +
>>> +               /* wait for DFLUSH cleared */
>>> +               readl_poll_timeout_atomic(&priv_dev->regs->ep_cmd, val,
>>> +                                         !(val & EP_CMD_DFLUSH), 1,
>>> + 1000);
>>> +
>>>                 link_trb->buffer = cpu_to_le32(TRB_BUFFER(priv_ep-
>>>trb_pool_dma +
>>>                         ((priv_req->end_trb + 1) * TRB_SIZE)));
>>>                 link_trb->control =
>>> cpu_to_le32((le32_to_cpu(link_trb->control) & TRB_CYCLE) | @@ -2660,6
>>> +2668,10 @@ int cdns3_gadget_ep_dequeue(struct usb_ep *ep,
>>>
>>>         cdns3_gadget_giveback(priv_ep, priv_req, -ECONNRESET);
>>>
>>> +       req = cdns3_next_request(&priv_ep->pending_req_list);
>>> +       if (req)
>>> +               cdns3_rearm_transfer(priv_ep, 1);
>>> +
>>
>>Why the above changes are needed?
>>
>
>Do you mean the last line or this patch?
>
>Last line:
>DMA is stopped, so driver arm the queued transfers
>
>If you means this patch:
>Issue was detected by customer test. I don’t know whether it was only test or
>the real application.
>
>The problem happens because user application queued the transfer (endpoint
>has been armed), so controller fetch the TRB.
>When user application removed this request the TRB was still processed by
>controller. If at that time the host will send data packet then controller will
>accept it, but it shouldn't because the usb_request associated with TRB cached
>by controller was removed.
>To force the controller to drop this TRB DFLUSH is required.
>
>Pawel
>
>>
>>>  not_found:
>>>         spin_unlock_irqrestore(&priv_dev->lock, flags);
>>>         return ret;
>>> --
>>> 2.25.1
>>>