linux-kernel - Re: [PATCH v2] 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] [day] [month] [year] [list]

Message-ID: <20250417113451.09ee5472@foxbook>
Date: Thu, 17 Apr 2025 11:34:51 +0200
From: Michał Pecio <michal.pecio@...il.com>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Mathias Nyman
 <mathias.nyman@...el.com>, Minas Harutyunyan <hminas@...opsys.com>, Linus
 Walleij <linus.walleij@...aro.org>, Alan Stern <stern@...land.harvard.edu>,
 linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] usb: hcd: Add a usb_device argument to
 hc_driver.endpoint_reset()

On Thu, 17 Apr 2025 11:54:19 +0300, Mathias Nyman wrote:
> On 15.4.2025 12.10, Michal Pecio wrote:
> > xHCI needs usb_device here, so it stored it in host_endpoint.hcpriv,
> > which proved problematic due to some unexpected call sequences from
> > USB core, and generally made the code more complex than it has to
> > be.
> > 
> > Make USB core supply it directly and simplify xhci_endpoint_reset().
> > Use the xhci_check_args() helper for preventing resets of emulated
> > root hub endpoints and for argument validation.
> > 
> > Update other drivers which also define such callback to accept the
> > new argument and ignore it, as it seems to be of no use for them.
> > 
> > This fixes a 6.15-rc1 regression reported by Paul, which I was able
> > to reproduce, where xhci_hcd doesn't handle endpoint_reset() after
> > endpoint_disable() not followed by add_endpoint(). If a configured
> > device is reset, stalling endpoints start to get stuck permanently.
> > 
> > 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>
> > ---  
> 
> All xhci changes look good to me
> 
> Acked-by: Mathias Nyman <mathias.nyman@...ux.intel.com>

Thank you for the review.

I guess I should update the commit message, though?

Technically, the regression will be closed by the next usb-linus merge
due to EP_STALLED reverts, while this patch really fixes and old hidden
bug which I could probably do a better job at explaining.

Greg would like a "Fixes". I think the problem started somewhere here:
f5249461b504 xhci: Clear the host side toggle manually when endpoint is soft reset
18b74067ac78 xhci: Fix use-after-free regression in xhci clear hub TT implementation

The former introduced an endpoint_reset() which depends on ep->hcpriv,
the latter introduced an endpoint_disable() which clears ep->hcpriv.
Which of them was wrong depends on whether it is legal to expect hcpriv
to be preserved after endpoint_disable(), I honestly don't know.

I also don't know if it will make sense to fix this in stable, since
nobody apparently noticed before EP_STALLED. But a class driver which
tries to clear a not halted EP on a device that had been reset in the
past could create toggle mismatch. I have not yet found such a driver.

Michal