Re: [PATCH] can: peak_pci: fix PCIeC cleanup on probe failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] can: peak_pci: UAF of pciec_card during probe failure
- [Low] can: peak_pci: misleading indentation in failure_free_dev
--

commit 97c9f38ab42de88035b4236926e467dfff4e2f5d
Author: Guangshuo Li <lgs201920130244@gmail.com>

can: peak_pci: fix PCIeC cleanup on probe failure

peak_pci_probe() initializes the PCIeC resources before registering the
SJA1000 network device.

Cc: stable@vger.kernel.org

> diff --git a/drivers/net/can/sja1000/peak_pci.c b/drivers/net/can/sja1000/peak_pci.c
> index 69c61ccf621df..28b94e6421709 100644
> --- a/drivers/net/can/sja1000/peak_pci.c
> +++ b/drivers/net/can/sja1000/peak_pci.c
> @@ -697,6 +697,8 @@ static int peak_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	return 0;
>  
>  failure_free_dev:
> +if (!chan->prev_dev && chan->pciec_card)
> +	peak_pciec_remove(chan->pciec_card);
>  	pci_set_drvdata(pdev, chan->prev_dev);
>  	free_sja1000dev(dev);
>  

[Severity: Low]
Does this misleading indentation violate kernel style guidelines?

The if condition is placed at column 0, while the pci_set_drvdata() and
free_sja1000dev() statements below it are indented with a tab. This visually
suggests they are conditionally executed by the if statement, even though
they run unconditionally.

> @@ -704,20 +706,18 @@ static int peak_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	/* Disable interrupts */
>  	writew(0x0, cfg_base + PITA_ICR + 2);
>  
> -	chan = NULL;
>  	for (dev = pci_get_drvdata(pdev); dev; dev = prev_dev) {
>  		priv = netdev_priv(dev);
>  		chan = priv->priv;
>  		prev_dev = chan->prev_dev;
>  
> +		/* do that only for first channel */
> +		if (!prev_dev && chan->pciec_card)
> +			peak_pciec_remove(chan->pciec_card);
>  		unregister_sja1000dev(dev);

[Severity: High]
Does this sequence result in a use-after-free of the card structure if device
registration fails for a secondary channel?

During the failure path, the driver iterates over previously registered
channels to unregister them. For the first channel, peak_pciec_remove() is
called, which frees the card structure.

Immediately following this, unregister_sja1000dev() is called, which places
the device into reset mode via priv->write_reg(). This resolves to
peak_pciec_write_reg(), which dereferences the freshly freed card structure:

drivers/net/can/sja1000/peak_pci.c:peak_pciec_write_reg() {
    ...
    int c = (priv->reg_base - card->reg_base) / PEAK_PCI_CHAN_SIZE;
    ...
}

Will this cause memory corruption or a crash?

Although the commit message notes this matches the ordering used by
peak_pci_remove(), it appears that function contains the same identical flaw
which is now being reproduced in peak_pci_probe().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915173157.2510375-1-lgs201920130244@gmail.com?part=1