On 8.10.2019 11.15, Mathias Nyman wrote:
On 7.10.2019 17.02, Johan Hovold wrote:
I didn't have time to look into this myself last week, or comment on the
patch before Greg picked it up, but this clearly isn't the right fix.
As your comment suggests, ep->hcpriv may indeed be NULL here if USB core
have allocated a new udev. But this only happens after USB has freed the
old usb_device and the new one happens to get the same address.
You're right, that fix doesn't solve the actual issue, it avoids a few specific
null pointer dereference cases, but leaves both root cause and several other
use-after-free cases open.
Note that even the usb_host_endpoint itself (ep) has then been freed and
reallocated since it is member of struct usb_device, and it is the
use-after-free that needs fixing.
I've even been able to trigger another NULL-deref in this function
before a new udev has been allocated, due to the virt dev having been
freed by xhci_free_dev as part of usb_release_dev:
It seems the xhci clear-tt implementation was incomplete since it did
not take care to wait for any ongoing work before disabling the
endpoint. EHCI does this in ehci_endpoint_disable(), but xhci doesn't
even implement that callback.
So it seems, it might be possible to remove pending clear_tt work for
most endpoints in the .drop_endpoint callbacks, but ep0 is different,
it isn't dropped, we might need to implement the endpoint_disable()
callback for this.
I was able to reproduce the use-after-free issue by faking a endpoint halt,
and resetting the endpoint early in enumeration at Get device descriptor request.
To fix this I added the endpoint_disable() callback that will wait for
clear_tt_work to finish before returning, similar to the ehci solution.
It works in my case.
the endpoint_disable() callback is called by usb core both before dropping
xhci endpoints belonging to a interface, and separately for ep0 before udev is
freed during enumeration retry.
Both the hack that triggers the issue (LS/FS behind HS bug won't ever enumerate with this)
and the fix to prevent use after free can be found in clear_tt_fix branch:
git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git clear_tt_fix
I'll send the fix out to the list as well, any chance you could try that out?
-Mathias
