Re: [PATCH net] can: peak_pci: Fix potential use-after-free during removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] can: sja1000: peak_pci: use-after-free in
  peak_pciec_led_work() [1]
--

commit c34b5385557b3189871973b5ef5794cd792b0b5e
Author: Guangshuo Li <liguangshuo@mail.sdu.edu.cn>

can: peak_pci: Fix potential use-after-free during removal

This commit moves unregister_sja1000dev() before peak_pciec_remove() in
peak_pci_remove() so that the ExpressCard context remains valid while the
first channel is unregistered.

> diff --git a/drivers/net/can/sja1000/peak_pci.c b/drivers/net/can/sja1000/peak_pci.c
> index 69c61ccf621df..618f8f6a57500 100644
> --- a/drivers/net/can/sja1000/peak_pci.c
> +++ b/drivers/net/can/sja1000/peak_pci.c
> @@ -752,10 +752,10 @@ static void peak_pci_remove(struct pci_dev *pdev)
>  		struct net_device *prev_dev = chan->prev_dev;
>  
>  		dev_info(&pdev->dev, "removing device %s\n", dev->name);
> +		unregister_sja1000dev(dev);
>  		/* do that only for first channel */
>  		if (!prev_dev && chan->pciec_card)
>  			peak_pciec_remove(chan->pciec_card);
> -		unregister_sja1000dev(dev);
>  		free_sja1000dev(dev);

[Severity: Critical]
This problem wasn't introduced by this patch, but can peak_pciec_led_work()
race with channel removal and cause a use-after-free [1] on
multi-channel ExpressCard devices?

peak_pci_remove() (and the failure_remove_channels unwind loop in
peak_pci_probe()) iterates over channels in reverse order, freeing
secondary channels via free_sja1000dev() while leaving their pointers in
card->channel[].  However, card->led_work is only cancelled by
peak_pciec_remove() once the loop reaches the first channel (!prev_dev):

CPU0                              CPU1
peak_pci_remove()
  free_sja1000dev(dev)
                                  peak_pciec_led_work()
                                    netdev = card->channel[i].netdev;
                                    if (!(netdev->flags & IFF_UP))
                                      <-- use-after-free
  if (!prev_dev && chan->pciec_card)
    peak_pciec_remove(chan->pciec_card)
      ...
        cancel_delayed_work_sync(&card->led_work)

Cancelling the LED work before freeing any channel netdev would avoid
this race.

[1] https://sashiko.dev/#/bug/linux-d81f042f-87f9-4ee0-9986-e44ae5a9d8a7

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-21df1234b2-can-peak-pci@mail.sdu.edu.cn?part=1