Messages in this thread | | | Date | Thu, 22 Dec 2022 13:08:47 +0200 | From | Mathias Nyman <> | Subject | Re: [PATCH v2] usb: xhci: Check endpoint is valid before dereferencing it |
| |
On 22.12.2022 11.01, Ladislav Michl wrote: > On Thu, Dec 22, 2022 at 07:29:12AM +0000, Jimmy Hu wrote: >> When the host controller is not responding, all URBs queued to all >> endpoints need to be killed. This can cause a kernel panic if we >> dereference an invalid endpoint. >> >> Fix this by using xhci_get_virt_ep() helper to find the endpoint and >> checking if the endpoint is valid before dereferencing it. >> >> [233311.853271] xhci-hcd xhci-hcd.1.auto: xHCI host controller not responding, assume dead >> [233311.853393] Unable to handle kernel NULL pointer dereference at virtual address 00000000000000e8 >> >> [233311.853964] pc : xhci_hc_died+0x10c/0x270 >> [233311.853971] lr : xhci_hc_died+0x1ac/0x270 >> >> [233311.854077] Call trace: >> [233311.854085] xhci_hc_died+0x10c/0x270 >> [233311.854093] xhci_stop_endpoint_command_watchdog+0x100/0x1a4 >> [233311.854105] call_timer_fn+0x50/0x2d4 >> [233311.854112] expire_timers+0xac/0x2e4 >> [233311.854118] run_timer_softirq+0x300/0xabc >> [233311.854127] __do_softirq+0x148/0x528 >> [233311.854135] irq_exit+0x194/0x1a8 >> [233311.854143] __handle_domain_irq+0x164/0x1d0 >> [233311.854149] gic_handle_irq.22273+0x10c/0x188 >> [233311.854156] el1_irq+0xfc/0x1a8 >> [233311.854175] lpm_cpuidle_enter+0x25c/0x418 [msm_pm] >> [233311.854185] cpuidle_enter_state+0x1f0/0x764 >> [233311.854194] do_idle+0x594/0x6ac >> [233311.854201] cpu_startup_entry+0x7c/0x80 >> [233311.854209] secondary_start_kernel+0x170/0x198 >> >> Fixes: 50e8725e7c42 ("xhci: Refactor command watchdog and fix split string.") >> Cc: stable@vger.kernel.org >> Signed-off-by: Jimmy Hu <hhhuuu@google.com> >> --- >> drivers/usb/host/xhci-ring.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c >> index ddc30037f9ce..f5b0e1ce22af 100644 >> --- a/drivers/usb/host/xhci-ring.c >> +++ b/drivers/usb/host/xhci-ring.c >> @@ -1169,7 +1169,10 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci, >> struct xhci_virt_ep *ep; >> struct xhci_ring *ring; >> >> - ep = &xhci->devs[slot_id]->eps[ep_index]; >> + ep = xhci_get_virt_ep(xhci, slot_id, ep_index); >> + if (!ep) >> + return; >> + > > xhci_get_virt_ep also adds check for slot_id == 0. It changes behaviour, > do we really want to skip that slot? Original code went from 0 to > MAX_HC_SLOTS-1. > > It seems to be off by one to me. Am I missing anything?
slot_id 0 is always invalid, so this is a good change.
> Also, what about passing ep directly to xhci_kill_endpoint_urbs > and do the check in xhci_hc_died? Not even compile tested:
passing ep to a function named kill_endpoint_urbs() sound like the right thing to do, but as a generic change.
I think its a good idea to first do a targeted fix for this null pointer issue that we can send to stable fist.
> > diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > index ddc30037f9ce..5dac483c562a 100644 > --- a/drivers/usb/host/xhci-ring.c > +++ b/drivers/usb/host/xhci-ring.c > @@ -1162,14 +1162,12 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, struct xhci_ring *ring) > } > > static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci, > - int slot_id, int ep_index) > + struct xhci_virt_ep *ep) > { > struct xhci_td *cur_td; > struct xhci_td *tmp; > - struct xhci_virt_ep *ep; > struct xhci_ring *ring; > > - ep = &xhci->devs[slot_id]->eps[ep_index]; > if ((ep->ep_state & EP_HAS_STREAMS) || > (ep->ep_state & EP_GETTING_NO_STREAMS)) { > int stream_id; > @@ -1180,18 +1178,12 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci, > if (!ring) > continue; > > - xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb, > - "Killing URBs for slot ID %u, ep index %u, stream %u", > - slot_id, ep_index, stream_id); > xhci_kill_ring_urbs(xhci, ring); > } > } else { > ring = ep->ring; > if (!ring) > return; > - xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb, > - "Killing URBs for slot ID %u, ep index %u", > - slot_id, ep_index); > xhci_kill_ring_urbs(xhci, ring); > } > > @@ -1217,6 +1209,7 @@ static void xhci_kill_endpoint_urbs(struct xhci_hcd *xhci, > void xhci_hc_died(struct xhci_hcd *xhci) > { > int i, j; > + struct xhci_virt_ep *ep; > > if (xhci->xhc_state & XHCI_STATE_DYING) > return; > @@ -1227,11 +1220,14 @@ void xhci_hc_died(struct xhci_hcd *xhci) > xhci_cleanup_command_queue(xhci); > > /* return any pending urbs, remove may be waiting for them */ > - for (i = 0; i <= HCS_MAX_SLOTS(xhci->hcs_params1); i++) { > + for (i = 0; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) { > if (!xhci->devs[i]) > continue; > - for (j = 0; j < 31; j++) > - xhci_kill_endpoint_urbs(xhci, i, j); > + for (j = 0; j < EP_CTX_PER_DEV; j++) { > + ep = &xhci->devs[i]->eps[j]; > + if (ep) > + xhci_kill_endpoint_urbs(xhci, ep); > + }
This does loop a bit more than the existing code. With this change its always HCS_MAX_SLOTS * EP_CTX_PER_DEV. Previously best case was just HCS_MAX_SLOTS.
-Mathias
| |