Messages in this thread | | | Date | Tue, 28 Nov 2023 15:57:51 +0200 | From | Mathias Nyman <> | Subject | Re: [PATCH] xhci: fix null pointer deref for xhci_urb_enqueue |
| |
On 20.11.2023 17.33, Mathias Nyman wrote: > On 18.11.2023 12.19, Kuen-Han Tsai wrote: >> Hi Mathias >> >>>> if (usb_endpoint_xfer_isoc(&urb->ep->desc)) >>>> @@ -1552,8 +1561,10 @@ static int xhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag >>>> num_tds = 1; >>>> >>>> urb_priv = kzalloc(struct_size(urb_priv, td, num_tds), mem_flags); >>> kzalloc with spinlock held, should preferably be moved outside lock, otherwise should use GFP_ATOMIC >> >> Thanks for pointing this out. I realize this patch is incorrect and it >> is non-ideal to include many codes unrelated to xhci->devs[slot_id] >> within the lock. >> >>> xhci_check_maxpacket() called here can't be called with spinlock held >> >> It appears that xhci_check_maxpacket() might potentially lead to a >> deadlock later if a spinlock is held. Is this the concern you were >> referring to? If not, please let me know if there are any other >> potential issues that I may have missed, thanks! > > xhci_check_maxpacket() will allocate memory, wait for completion, and use the same lock, > so there are several issues here. > > I actually think we shouldn't call xhci_check_maxpacket() at all while queuing urbs. > > usb core knows when there was max packet size mismatch during enumeration. > I think we should add a hook to the hcd that usb core can call in these cases
I moved the max packet checks away from xhci_urb_enqueue() and fixed up the locking.
I can't trigger the original issue, but I tested it by setting incorrect initial max packet size values.
If you have the chance to test this with your setup I'd appreciate it.
patches found here: git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git fix_urb_enqueue_locking https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=fix_urb_enqueue_locking
I'll add them to this thread as well
thanks Mathias
| |