* [bug report] xhci: add support to allocate several interrupters
@ 2024-01-10 11:31 Dan Carpenter
2024-01-10 12:55 ` Mathias Nyman
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2024-01-10 11:31 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb
Hello Mathias Nyman,
The patch c99b38c41234: "xhci: add support to allocate several
interrupters" from Jan 2, 2024 (linux-next), leads to the following
Smatch static checker warning:
drivers/usb/host/xhci-mem.c:1873 xhci_remove_secondary_interrupter()
error: we previously assumed 'ir' could be null (see line 1865)
drivers/usb/host/xhci-mem.c
1863
1864 /* interrupter 0 is primary interrupter, don't touch it */
1865 if (!ir || !ir->intr_num || ir->intr_num >= xhci->max_interrupters)
1866 xhci_dbg(xhci, "Invalid secondary interrupter, can't remove\n");
This debug message is kind of pointless... The Oops will have the stack
trace already.
1867
1868 /* fixme, should we check xhci->interrupter[intr_num] == ir */
1869 /* fixme locking */
1870
1871 spin_lock_irq(&xhci->lock);
1872
--> 1873 intr_num = ir->intr_num;
^^^^^^^^^^^^
Unchecked dereference
1874
1875 xhci_remove_interrupter(xhci, ir);
1876 xhci->interrupters[intr_num] = NULL;
1877
1878 spin_unlock_irq(&xhci->lock);
1879
1880 xhci_free_interrupter(xhci, ir);
1881 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] xhci: add support to allocate several interrupters
2024-01-10 11:31 [bug report] xhci: add support to allocate several interrupters Dan Carpenter
@ 2024-01-10 12:55 ` Mathias Nyman
0 siblings, 0 replies; 6+ messages in thread
From: Mathias Nyman @ 2024-01-10 12:55 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-usb
Hi
On 10.1.2024 13.31, Dan Carpenter wrote:
> Hello Mathias Nyman,
>
> The patch c99b38c41234: "xhci: add support to allocate several
> interrupters" from Jan 2, 2024 (linux-next), leads to the following
> Smatch static checker warning:
>
> drivers/usb/host/xhci-mem.c:1873 xhci_remove_secondary_interrupter()
> error: we previously assumed 'ir' could be null (see line 1865)
>
> drivers/usb/host/xhci-mem.c
> 1863
> 1864 /* interrupter 0 is primary interrupter, don't touch it */
> 1865 if (!ir || !ir->intr_num || ir->intr_num >= xhci->max_interrupters)
> 1866 xhci_dbg(xhci, "Invalid secondary interrupter, can't remove\n");
>
> This debug message is kind of pointless... The Oops will have the stack
> trace already.
Ah, there's a return statement missing here.
Thanks for the report
Mathias
^ permalink raw reply [flat|nested] 6+ messages in thread
* [bug report] xhci: add support to allocate several interrupters
@ 2024-01-10 13:15 Dan Carpenter
2024-01-10 14:03 ` Mathias Nyman
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2024-01-10 13:15 UTC (permalink / raw)
To: mathias.nyman; +Cc: linux-usb
Hello Mathias Nyman,
The patch c99b38c41234: "xhci: add support to allocate several
interrupters" from Jan 2, 2024 (linux-next), leads to the following
Smatch static checker warning:
drivers/usb/host/xhci-mem.c:2331 xhci_add_interrupter()
warn: array off by one? 'xhci->interrupters[intr_num]'
drivers/usb/host/xhci-mem.c
2318 static int
2319 xhci_add_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir,
2320 unsigned int intr_num)
2321 {
2322 u64 erst_base;
2323 u32 erst_size;
2324
2325 if (intr_num > xhci->max_interrupters) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This check predates your commit.
2326 xhci_warn(xhci, "Can't add interrupter %d, max interrupters %d\n",
2327 intr_num, xhci->max_interrupters);
2328 return -EINVAL;
2329 }
2330
--> 2331 if (xhci->interrupters[intr_num]) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
But, yeah, it's an off by one. This is allocated in xhci_mem_init().
2332 xhci_warn(xhci, "Interrupter %d\n already set up", intr_num);
2333 return -EINVAL;
2334 }
2335
2336 xhci->interrupters[intr_num] = ir;
2337 ir->intr_num = intr_num;
2338 ir->ir_set = &xhci->run_regs->ir_set[intr_num];
However, potentially there was already a bug here? Normally "max" type
names are inclusive and "number" or "count" type names are the count.
So maybe > xhci->max_interrupters was correct and we should allocated 1
more element. But the code is kind of mixed with regards to whether
it's a max or a count and I can't be sure one way or the other.
2339
2340 /* set ERST count with the number of entries in the segment table */
2341 erst_size = readl(&ir->ir_set->erst_size);
2342 erst_size &= ERST_SIZE_MASK;
2343 erst_size |= ir->event_ring->num_segs;
2344 writel(erst_size, &ir->ir_set->erst_size);
2345
2346 erst_base = xhci_read_64(xhci, &ir->ir_set->erst_base);
2347 erst_base &= ERST_BASE_RSVDP;
2348 erst_base |= ir->erst.erst_dma_addr & ~ERST_BASE_RSVDP;
2349 xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base);
2350
2351 /* Set the event ring dequeue address of this interrupter */
2352 xhci_set_hc_event_deq(xhci, ir);
2353
2354 return 0;
2355 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [bug report] xhci: add support to allocate several interrupters
2024-01-10 13:15 Dan Carpenter
@ 2024-01-10 14:03 ` Mathias Nyman
2024-01-10 14:54 ` Mathias Nyman
0 siblings, 1 reply; 6+ messages in thread
From: Mathias Nyman @ 2024-01-10 14:03 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-usb
Hi
On 10.1.2024 15.15, Dan Carpenter wrote:
> Hello Mathias Nyman,
>
> The patch c99b38c41234: "xhci: add support to allocate several
> interrupters" from Jan 2, 2024 (linux-next), leads to the following
> Smatch static checker warning:
>
> drivers/usb/host/xhci-mem.c:2331 xhci_add_interrupter()
> warn: array off by one? 'xhci->interrupters[intr_num]'
>
> drivers/usb/host/xhci-mem.c
> 2318 static int
> 2319 xhci_add_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir,
> 2320 unsigned int intr_num)
> 2321 {
> 2322 u64 erst_base;
> 2323 u32 erst_size;
> 2324
> 2325 if (intr_num > xhci->max_interrupters) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This check predates your commit.
Yes, this is incorrect.
Should be intr_num >= xhci->max_interrupters.
>
>
> 2326 xhci_warn(xhci, "Can't add interrupter %d, max interrupters %d\n",
> 2327 intr_num, xhci->max_interrupters);
> 2328 return -EINVAL;
> 2329 }
> 2330
> --> 2331 if (xhci->interrupters[intr_num]) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> But, yeah, it's an off by one. This is allocated in xhci_mem_init().
>
>
> 2332 xhci_warn(xhci, "Interrupter %d\n already set up", intr_num);
> 2333 return -EINVAL;
> 2334 }
> 2335
> 2336 xhci->interrupters[intr_num] = ir;
> 2337 ir->intr_num = intr_num;
> 2338 ir->ir_set = &xhci->run_regs->ir_set[intr_num];
>
> However, potentially there was already a bug here? Normally "max" type
> names are inclusive and "number" or "count" type names are the count.
> So maybe > xhci->max_interrupters was correct and we should allocated 1
> more element. But the code is kind of mixed with regards to whether
> it's a max or a count and I can't be sure one way or the other.
There was a bug, intr_num is basically an array index.
Luckily this doesn't cause any real word harm (yet) as xhci_add_interrupter()
is currently always called with a intr_num value smaller than xhci->max_interrupters.
git grep -B 3 "xhci_add_interrupter(x"
xhci-mem.c- /* Find available secondary interrupter, interrupter 0 is reserved for primary */
xhci-mem.c- for (i = 1; i < xhci->max_interrupters; i++) {
xhci-mem.c- if (xhci->interrupters[i] == NULL) {
xhci-mem.c: err = xhci_add_interrupter(xhci, ir, i);
--
xhci-mem.c- if (!ir)
xhci-mem.c- goto fail;
xhci-mem.c-
xhci-mem.c: if (xhci_add_interrupter(xhci, ir, 0))
Thanks
Mathias
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-10 14:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-10 11:31 [bug report] xhci: add support to allocate several interrupters Dan Carpenter
2024-01-10 12:55 ` Mathias Nyman
-- strict thread matches above, loose matches on Subject: below --
2024-01-10 13:15 Dan Carpenter
2024-01-10 14:03 ` Mathias Nyman
2024-01-10 14:54 ` Mathias Nyman
2024-01-10 14:56 ` Mathias Nyman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.