linux-kernel - Re: [PATCH] usb: dwc3: Skip TRBs while removing requests in disconnect path

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: <20230601012953.47xh7meyr2woowpc@synopsys.com>
Date:   Thu, 1 Jun 2023 01:30:02 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Elson Serrao <quic_eserrao@...cinc.com>
CC:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "quic_wcheng@...cinc.com" <quic_wcheng@...cinc.com>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>
Subject: Re: [PATCH] usb: dwc3: Skip TRBs while removing requests in
 disconnect path

On Wed, May 31, 2023, Elson Serrao wrote:
> 
> 
> On 5/31/2023 4:20 PM, Thinh Nguyen wrote:
> > On Wed, May 31, 2023, Elson Roy Serrao wrote:
> > > Consider a scenario where cable disconnect happens when there is an active
> > > usb reqest queued to the UDC. As part of the disconnect we would issue an
> > > end transfer with no interrupt-on-completion before giving back this
> > > request. Since we are giving back the request without skipping TRBs the
> > > num_trbs field of dwc3_request still holds the stale value previously used.
> > > Function drivers re-use same request for a given bind-unbind session and
> > > hence their dwc3_request context gets preserved across cable
> > > disconnect/connect. When such a request gets re-queued after cable connect,
> > 
> > Why would we preserve the request after a disconnect? The request is
> > associated with an endpoint, and after disconnect, the endpoint is no
> > longer valid. Shouldn't the request be freed then?
> > 
> 
> 
> Function drivers generally allocate usb requests during bind when an
> endpoint is allocated to it (through usb_ep_autoconfig). These requests are
> freed when an unbind is called as the function is no longer associated with
> any end point. The function driver is free to re-use these requests
> throughout this bind-unbind session. The only restriction is that the
> function drivers wont be able to queue any requests as long as the endpoint

> is disabled. But that doesn't enforce function drivers to free the requests
> with ep_disable(). Even though the endpoint is disabled with cable
> disconnect, that endpoint is still associated with that particular function
> driver until that function is unbound.
> 
> As an example below is how f_ncm driver allocates and frees the requests
> during bind/unbind
> 
> Bind()
> ...
> ep = usb_ep_autoconfig(cdev->gadget, &fs_ncm_notify_desc);
> if (!ep)
> 	goto fail;
> ncm->notify = ep;
> 
> status = -ENOMEM;
> 
> /* allocate notification request and buffer */
> ncm->notify_req = usb_ep_alloc_request(ep, GFP_KERNEL);
> ...
> 
> The endpoint is enabled later when set_alt is received and disabled in
> ncm_disable when the connection goes down (cable disconnect scenario)
> 
> 
> Unbind()
> ....
> kfree(ncm->notify_req->buf);
> usb_ep_free_request(ncm->notify, ncm->notify_req);
> 
> I see similar implementation in other function drivers as well. That is,
> keep the usb requests allocated throughout the bind-unbind session and
> independent of ep_enable/ep_disable .
> 
> Thanks
> Elson
> 

Thanks for the clarification. Then you just need to reset the num_trbs
count when giving back the request. Can we do that in
dwc3_gadget_del_and_unmap_request()?

Please add a fix tag.

Thanks,
Thinh