[bug report] xhci: add support to allocate several interrupters

[bug report] xhci: add support to allocate several interrupters

[Dan Carpenter]

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

Re: [bug report] xhci: add support to allocate several interrupters

[Mathias Nyman]

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

[bug report] xhci: add support to allocate several interrupters

[Dan Carpenter]

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

Re: [bug report] xhci: add support to allocate several interrupters

[Mathias Nyman]

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

Re: [bug report] xhci: add support to allocate several interrupters

[Mathias Nyman]

On 10.1.2024 16.03, Mathias Nyman wrote:
> 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:
>>

Patches for both cases you reported are not in my for-usb-linus branch:

git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=for-usb-linus

Once 6.8-rc1 is out I'll rebase on top of it and send them to the list.

Thanks
Mathias

Re: [bug report] xhci: add support to allocate several interrupters

[Mathias Nyman]

On 10.1.2024 16.54, Mathias Nyman wrote:
> On 10.1.2024 16.03, Mathias Nyman wrote:
>> 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:
>>>
> 
> Patches for both cases you reported are not in my for-usb-linus branch:

are _now_ in my for-usb_linus branch

> 
> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
> https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=for-usb-linus
> 
> Once 6.8-rc1 is out I'll rebase on top of it and send them to the list.
> 
> Thanks
> Mathias