| Message ID | 20241219215519.273c5c41@foxbook (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] usb: xhci: Fix NULL pointer dereference on certain command aborts | expand |
On 19.12.2024 22.55, Michal Pecio wrote: > 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. 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. > > 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. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=219532 > Fixes: c311e391a7ef ("xhci: rework command timeout and cancellation,") > CC: stable@vger.kernel.org > Signed-off-by: Michal Pecio <michal.pecio@gmail.com> > --- Adding to queue Thanks -Mathias
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index 09b05a62375e..dfe1a676d487 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -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); } }
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. Link: https://bugzilla.kernel.org/show_bug.cgi?id=219532 Fixes: c311e391a7ef ("xhci: rework command timeout and cancellation,") CC: stable@vger.kernel.org Signed-off-by: Michal Pecio <michal.pecio@gmail.com> --- 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(-)