Re: [PATCH v2] usb: xhci: Check endpoint is valid before dereferencing it

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx
Signed-off-by: Jimmy Hu <hhhuuu@xxxxxxxxxx>
---
  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





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux