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
