linux-kernel - Re: [PATCH RFC RFT] usb: hcd: Add a usb_device argument to hc_driver.endpoint

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: <357368ff-0c49-4f22-a03d-fd9560c22dae@linux.intel.com>
Date: Tue, 8 Apr 2025 16:55:24 +0300
From: Mathias Nyman <mathias.nyman@...ux.intel.com>
To: Michał Pecio <michal.pecio@...il.com>,
 Alan Stern <stern@...land.harvard.edu>,
 Mathias Nyman <mathias.nyman@...el.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Paul Menzel <pmenzel@...gen.mpg.de>, linux-usb@...r.kernel.org,
 LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC RFT] usb: hcd: Add a usb_device argument to
 hc_driver.endpoint_reset()

On 8.4.2025 13.18, Michał Pecio wrote:
> xHCI needs usb_device in this callback so it employed some hacks that
> proved unreliable in the long term and made the code a little tricky.
> 
> Make USB core supply it directly and simplify xhci_endpoint_reset().
> Use xhci_check_args() to prevent resetting emulated endpoints of root
> hubs and to deduplicate argument validation and improve debuggability.
> 
> Update ehci_endpoint_reset(), which is the only other such callback,
> to accept (and ignore) the new argument.
> 
> This fixes the root cause of a 6.15-rc1 regression reported by Paul,
> which I was able to reproduce locally. It also solves the general
> problem of xhci_endpoint_reset() becoming a no-op after device reset
> or changing configuration or altsetting. Although nobody complained
> because halted endpoints are reset automatically by xhci_hcd, it was
> a bug - sometimes class drivers want to reset not halted endpoints.
> 
> Reported-by: Paul Menzel <pmenzel@...gen.mpg.de>
> Closes: https://lore.kernel.org/linux-usb/c279bd85-3069-4841-b1be-20507ac9f2d7@molgen.mpg.de/
> Signed-off-by: Michal Pecio <michal.pecio@...il.com>
> ---
> 
> Is such change acceptable to interested parties?

Looks like an improvement, and will help clear the EP_STALLED flag
eventually in device reset case.

There are some issues still unsolved due to how xhci endpoints end up being handled
in usb core usb_reset_and_verify_device().

usb_reset_and_verify_device()
   hub_port_init()
     hcd->driver->reset_device(hcd, udev);          /*1 xhci frees ep rings, loses td_list heads */
   usb_hcd_alloc_bandwidth(new_config, NULL, NULL)  /*2 xhci drop+add ep, allocated new ep rings */
   usb_control_msg(udev, usb_sndctrlpipe(...,USB_REQ_SET_CONFIGURATION,...)
   for (interfaces) {
     if (AlternateSetting == 0) {
       usb_disable_interface()  /*3 flush urbs, ->xhci_urb_dequeue() */
       usb_enable_interface()   /*4 clear EP_STALLED flag */
     } else {
       usb_set_interface()
     }

1. driver->reset_device will free all xhci endpoint rings, and lose td_list head, but
    keep cancelled_td_list and ep->ep_state flags. xHC issues reset device command
    setting all internal ep states in xci to "disabled".

2. usb hcd_alloc_bandwith will drop+add xhci endpoints for this configuration,
    allocate new endpoint rings, and inits new td_list head.
    Old cancelled_td_list and ep_state flags are still set, not matching ring.

3. usb_disable_interface() will flush all pending URBs calling xhci_urb_dequeue().
    xhci_urb_dequeue() makes decision based on stale ep_state flags.
    May start to cancel/giveback urbs in old cancelled_td_list for tds that existed
    on old freed ring. will also set host_ep->hcpriv to null

4. usb_enable_interface() calls xhci_endpoint_reset() that finally clears
    the EP_STALLED flag (udev now found thanks to this patch)

Disabling endpoints, like calling usb_disable_interface() in step 3 should be
done before calling  usb_hcd_alloc_bandwith().
This was fixed in other places such as usb_set_interface() and usb_reset_config()

We might need to clean up ep_state flags and give back cancelled URBs in
xhci_discover_or_reset_device() after the reset device xhci command completion.

Thanks
Mathias