Re: [PATCH net] can: ctucanfd: unmap BAR0 using base address

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] NAPI poll livelock on SKB allocation failure.
- [High] Missing bounds checks on PCI BAR lengths before MMIO accesses.
--

--- Patch [1]: [PATCH net] can: ctucanfd: unmap BAR0 using base address ---
commit 6b71833578fcc5fa2bdb08928e4e80211ffbd177
Author: Avi Weiss <[email protected]>

can: ctucanfd: unmap BAR0 using base address

BAR0 is mapped into bar0_base, while cra_addr points to an offset within
that mapping and is used for other purposes. This patch passes bar0_base
to pci_iounmap(), instead of cra_addr, on the probe error path so the
address returned by pci_iomap() is used for unmapping.

> diff --git a/drivers/net/can/ctucanfd/ctucanfd_pci.c b/drivers/net/can/ctucanfd/ctucanfd_pci.c
> index 625788fa89766..f845951c5d513 100644
> --- a/drivers/net/can/ctucanfd/ctucanfd_pci.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_pci.c
> @@ -194,7 +194,7 @@ static int ctucan_pci_probe(struct pci_dev *pdev,
>  	pci_set_drvdata(pdev, NULL);
>  	kfree(bdata);
>  err_pci_iounmap_bar0:
> -	pci_iounmap(pdev, cra_addr);
> +	pci_iounmap(pdev, bar0_base);
>  err_pci_iounmap_bar1:
>  	pci_iounmap(pdev, addr);
>  err_release_regions:

While reviewing this patch, I noticed a couple of pre-existing issues in
the driver.

> @@ ... @@ static int ctucan_pci_probe(struct pci_dev *pdev,
>  	if (driver_data == CTUCAN_WITHOUT_CTUCAN_ID) {
>  		cra_addr = bar0_base;
>  		num_cores = 2;
>  	} else {
>  		cra_addr = bar0_base + CTUCAN_BAR0_CRA_BASE;
>  		ctucan_id = ioread32(bar0_base + CTUCAN_BAR0_CTUCAN_ID);
>  		dev_info(dev, "ctucan_id 0x%08lx\n", (unsigned long)ctucan_id);
>  		num_cores = ctucan_id & 0xf;
>  	}
[ ... ]
>  	while (core_i < num_cores) {
>  		addr += 0x4000;
>  		ret = ctucan_probe_common(dev, addr, irq, ntxbufs, 100000000,
>  					  0, ctucan_pci_set_drvdata);

[Severity: High]
This isn't a bug introduced by this patch, but does this code lack bounds
checking on the PCI BAR lengths?

The driver extracts num_cores from a hardware register, which could allow
up to 15 cores.  Then it advances the MMIO pointer 'addr' out-of-bounds by
0x4000 for each core and probes the device based on this untrusted value.

Can a malicious or malfunctioning PCIe device (or virtual device via
passthrough) advertise a small BAR size but a large num_cores, triggering
an out-of-bounds MMIO access and kernel panic?


> diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
> --- a/drivers/net/can/ctucanfd/ctucanfd_base.c
> +++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
> @@ ... @@ static int ctucan_rx(struct net_device *ndev)
>  	if (unlikely(!skb)) {
>  		priv->rxfrm_first_word = ffw;
>  		set_bit(CTUCANFD_FLAG_RX_FFW_BUFFERED, &priv->drv_flags);
>  		return 0;
>  	}
[ ... ]
> @@ ... @@ static int ctucan_rx_poll(struct napi_struct *napi, int quota)
>  	if (!framecnt && res != 0) {
>  		if (napi_complete_done(napi, work_done)) {
>  			/* Clear and enable RBNEI. It is level-triggered, so
>  			 * there is no race condition.
>  			 */

[Severity: High]
This is a pre-existing issue, but could this cause a NAPI poll livelock
on SKB allocation failure?

If alloc_can_skb() or alloc_canfd_skb() fails due to OOM in ctucan_rx(),
it returns 0.

The NAPI poll routine ctucan_rx_poll() then skips napi_complete_done()
because res == 0, causing the !framecnt && res != 0 check to evaluate to
false.

Because it returns less than quota without calling napi_complete_done(),
doesn't this violate NAPI semantics?  The core will immediately reschedule
the poll, creating an infinite loop trying to allocate memory and leading
to 100% CPU usage and a potential system lockup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1