Re: [PATCH net 11/14] can: kvaser_pciefd: fix use-after-free in bec poll timer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing dma_rmb() when reading packets from coherent DMA memory.
- [High] Incomplete timer teardown and missing state checks in ndo_stop (kvaser_pciefd_stop) causes a polling loop leak, state corruption, and potential spurious netif_rx calls when the interface is brought down.
--

commit d6148cc43f3a9adbcbc3d9812cbf816bc0ffca1f
Author: Fan Wu <fanwu01@zju.edu.cn>

can: kvaser_pciefd: fix use-after-free in bec poll timer

This commit addresses a use-after-free by switching to timer_shutdown_sync()
in kvaser_pciefd_remove() and kvaser_pciefd_teardown_can_ctrls(). It ensures
the bec_poll_timer callback completes before freeing the CAN device during
module removal or probe errors.

Link: https://patch.msgid.link/20260818063832.383829-1-fanwu01@zju.edu.cn

> diff --git a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
> index d8c9bfb202304..a0597db720864 100644
> --- a/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c
> +++ b/drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c

[ ... ]

> @@ -1879,7 +1880,7 @@ static void kvaser_pciefd_remove(struct pci_dev *pdev)
>  		struct kvaser_pciefd_can *can = pcie->can[i];
>  
>  		unregister_candev(can->can.dev);
> -		timer_delete(&can->bec_poll_timer);
> +		timer_shutdown_sync(&can->bec_poll_timer);

[Severity: High]
This is a pre-existing issue, but since you are fixing the timer teardown here,
does the timer also need to be synced during ndo_stop?

In kvaser_pciefd_stop(), the code uses the non-sync timer_delete:

	iowrite32(0, can->reg_base + KVASER_PCIEFD_KCAN_IEN_REG);
	timer_delete(&can->bec_poll_timer);
	can->can.state = CAN_STATE_STOPPED;

Because this does not wait for the callback to finish, couldn't a pending status
request be issued to the hardware right as the interface is brought down?

Furthermore, if a status response arrives via the shared PCIe interrupt after
the interface is stopped, kvaser_pciefd_handle_status_packet() and
kvaser_pciefd_handle_status_resp() do not seem to check if the device was
intentionally stopped. They overwrite the state to an active state and
unconditionally re-arm the timer:

drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c:kvaser_pciefd_handle_status_packet() {
    ...
	if (can->can.state != CAN_STATE_BUS_OFF &&
	    can->can.state != CAN_STATE_ERROR_ACTIVE) {
		mod_timer(&can->bec_poll_timer, KVASER_PCIEFD_BEC_POLL_FREQ);
	}
    ...
}

Could this lead to an infinite polling loop leak and potential spurious
netif_rx calls on a stopped interface?

[Severity: Critical]
This is also a pre-existing issue, but I noticed a potential memory ordering
issue in kvaser_pciefd_read_packet().

When reading from the coherent DMA buffer:

drivers/net/can/kvaser_pciefd/kvaser_pciefd_core.c:kvaser_pciefd_read_packet() {
    ...
	size = le32_to_cpu(buffer[pos++]);
	if (!size) {
		*start_pos = 0;
		return 0;
	}

	p->header[0] = le32_to_cpu(buffer[pos++]);
    ...
}

Is a dma_rmb() needed before reading the packet contents? 

Without a read memory barrier, weakly ordered CPU architectures might
speculatively read the header and payload before the size read completes.
If the hardware is actively updating the buffer, couldn't the CPU see the
new size but process stale header or payload data that was speculatively
prefetched?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=11