usb: xhci: Fix NULL pointer dereference on certain command aborts

Hi,

This fixes an issue discovered by code review prompted by this crash:
https://bugzilla.kernel.org/show_bug.cgi?id=219532

It has not been confirmed by the reporter and doesn't agree with some of his own analysis, but:

1. it is consistent with logs provided by the reporter
2. it looks like a real bug in its own right anyway
3. it is rare enough that it may not happen again soon

I tried to minimize this patch and only address the potential crash. The code looks like it could use more cleanup, though. Any comments regarding the following suggestions?

1. It looks like we don't really need to ring the doorbell if cmd_list and cur_cmd are empty. Even if commands were no-oped, they are still on the list. And if there is anything between enqueue and dequeue which is not a link TRB and not on the list, we will have no idea how to handle the resulting event. IOW, it's not supposed to happen and probably never happening, or we would hear about it.

2. The whole business about turning commands into no-ops doesn't seem to happen in mainline today. Only the pending command gets aborted and it is removed from the list before the no-op loop runs.

3. xhci->current_cmd seems redundant - is there ever a valid reason for it being anything other than xhci->cmd_list first element?

I find it suspicious that handle_cmd_completion() complains if the completed command isn't the first on cmd_list, but not if it doesn't match current_cmd and that current_cmd is then left unchanged.

Regards,
Michal

xhci_handle_stopped_cmd_ring() attempts to restart the command ring if there are pending TRBs on it, which it detects by comparing the enqueue and dequeue pointers for equality. It assumes that pending TRBs imply a pending command, and blindly dereferences cur_cmd. This can be wrong.

If a command is queued to the final usable TRB of a ring segment, the enqueue pointer is advanced to the subsequent link TRB and no further. If the command is later aborted, when the abort completion is handled the dequeue pointer is advanced to the first TRB of the next segment.

If no further commands are queued, the pointers stay this way and then xhci_handle_stopped_cmd_ring() is called by xhci_abort_cmd_ring() with NULL cur_cmd, which triggers cur_cmd dereference as described above.

Fix this by omitting timer setup if cur_cmd is NULL. Leave the rest unchanged, including ringing the doorbell each time the ring pointers aren't equal. An unnecessary doorbell ring should be harmless.

This is probably Bug 219532, but no confirmation has been received.

---
 drivers/usb/host/xhci-ring.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 4cf5363875c7..da26e317ab0c 100644
@@ -422,7 +422,8 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
 	if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
 	    !(xhci->xhc_state & XHCI_STATE_DYING)) {
 		xhci->current_cmd = cur_cmd;
-		xhci_mod_cmd_timer(xhci);
+		if (cur_cmd)
+			xhci_mod_cmd_timer(xhci);
 		xhci_ring_cmd_db(xhci);
 	}
 }

2.43.0

I confirmed that the bug is real and behaves exactly as expected, using a USB microcontroller programmed to NAK the status stage of SET_ADDRESS requests forever and to reconnect if the host gives up enumerating it.

Command timeout was reduced to 500ms to sooner reach the segment's end and some relevant debug info was added, hopefully self-explanatory:

[  +0,378926] usb 10-1: new full-speed USB device number 109 using xhci_hcd
[  +0,501006] xhci_hcd 0000:03:00.0: cur_cmd 0000000000000000 enq ffff88814671bff0 deq ffff88814671b000
[  +0,000001] xhci_hcd 0000:03:00.0: Timeout while waiting for setup device command
[  +0,000005] xhci_hcd 0000:03:00.0: !!! avoiding dereferencing a NULL pointer !!!
[  +0,712001] xhci_hcd 0000:03:00.0: cur_cmd 0000000000000000 enq ffff88814671b010 deq ffff88814671b010
[  +0,000001] xhci_hcd 0000:03:00.0: Timeout while waiting for setup device command
[  +0,207981] usb 10-1: device not accepting address 109, error -62

The driver and host controller continue working normally after one hour of testing and several avoided crashes.

The only thing I haven't tried is actually crashing the kernel, but considering what's inside xhci_mod_cmd_timer() I think it's obvious that this is exactly what would happen next without this patch.

Regards,
Michal

If a command is queued to the final usable TRB of a ring segment, the enqueue pointer is advanced to the subsequent link TRB and no further. If the command is later aborted, when the abort completion is handled the dequeue pointer is advanced to the first TRB of the next segment.

If no further commands are queued, xhci_handle_stopped_cmd_ring() sees the ring pointers unequal and assumes that there is a pending command, so it calls xhci_mod_cmd_timer() which crashes if cur_cmd was NULL.

Don't attempt timer setup if cur_cmd is NULL. The subsequent doorbell ring likely is unnecessary too, but it's harmless. Leave it alone.

This is probably Bug 219532, but no confirmation has been received.

The issue has been independently reproduced and confirmed fixed using a USB MCU programmed to NAK the Status stage of SET_ADDRESS forever. Everything continued working normally after several prevented crashes.

---

This is practically a RESEND of a patch submitted earlier this month, but with updated commit message.

 drivers/usb/host/xhci-ring.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 09b05a62375e..dfe1a676d487 100644
@@ -422,7 +422,8 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
 	if ((xhci->cmd_ring->dequeue != xhci->cmd_ring->enqueue) &&
 	    !(xhci->xhc_state & XHCI_STATE_DYING)) {
 		xhci->current_cmd = cur_cmd;
-		xhci_mod_cmd_timer(xhci);
+		if (cur_cmd)
+			xhci_mod_cmd_timer(xhci);
 		xhci_ring_cmd_db(xhci);
 	}
 }

2.47.1

Nice catch.

That enqueue-dequeue pointer comparison should probably be changed to checking cmd_list directly. We do use list_empty() and list_is_singular() in other places

But that would be a separate cleanup later.

Adding to queue

Thanks
-Mathias