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

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

Pre-existing issues:
- [High] The driver uses non-synchronous `timer_delete()` when stopping the device or initializing the bus, allowing the timer callback to execute concurrently with hardware teardown/startup.
- [High] The sequence counter `can->cmd_seq` is incremented concurrently from hardirq, softirq, and process contexts without any synchronization or atomic types, leading to data corruption.
- [Medium] Error handling in `kvaser_pciefd_setup_can_ctrls()` calls `kvaser_pciefd_devlink_port_unregister()` on a port that failed to register, triggering a debug splat on uninitialized delayed work.
--

--- Patch [1]: [PATCH] can: kvaser_pciefd: fix use-after-free in bec poll timer ---
commit b386356ff6c7a43deaac5d1df270183cf372b998
Author: Fan Wu <[email protected]>

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

This patch replaces timer_delete() with timer_shutdown_sync() during
device teardown to prevent a use-after-free if the bec poll timer is
concurrently re-arming or running. It also adds the timer shutdown to
the probe error path.

> 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
> @@ -1739,6 +1739,7 @@ static void kvaser_pciefd_teardown_can_ctrls(struct kvaser_pciefd *pcie)
>  			iowrite32(0, can->reg_base + KVASER_PCIEFD_KCAN_IEN_REG);
>  			kvaser_pciefd_pwm_stop(can);
>  			kvaser_pciefd_devlink_port_unregister(can);

[Severity: Medium]
This is a pre-existing issue, but since we are modifying this error path,
could this unregister call trigger a debug splat on uninitialized delayed
work?

If kvaser_pciefd_devlink_port_register() fails in
kvaser_pciefd_setup_can_ctrls():

kvaser_pciefd_setup_can_ctrls() {
    ...
    pcie->can[i] = can;
    kvaser_pciefd_pwm_start(can);
    ret = kvaser_pciefd_devlink_port_register(can);
    if (ret)
        goto err;
    ...
err:
    kvaser_pciefd_teardown_can_ctrls(pcie);
}

Since pcie->can[i] is already populated, teardown unconditionally calls
kvaser_pciefd_devlink_port_unregister(can) on the failed port.

> +			timer_shutdown_sync(&can->bec_poll_timer);
>  			free_candev(can->can.dev);
>  		}
>  	}
> @@ -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 isn't a bug introduced by this patch, but since we are fixing the
timer cancellation here, are there similar concurrent execution risks in
other parts of the driver?

The driver still uses non-synchronous timer_delete() when initializing
the bus and stopping the device:

kvaser_pciefd_bus_on() {
    ...
    timer_delete(&can->bec_poll_timer);
    ...
}

kvaser_pciefd_stop() {
    ...
    timer_delete(&can->bec_poll_timer);
    ...
}

If the interface is brought down (triggering kvaser_pciefd_stop) while
polling is active, could the callback still execute concurrently and
re-arm the timer or modify hardware registers out of order?

>  		kvaser_pciefd_pwm_stop(can);
>  		kvaser_pciefd_devlink_port_unregister(can);
>  	}

[ ... ]

[Severity: High]
This is also a pre-existing issue not directly related to the lines changed
in this patch, but does the sequence counter increment in
kvaser_pciefd_send_kcan_cmd() need synchronization?

kvaser_pciefd_send_kcan_cmd() {
    ...
    FIELD_PREP(KVASER_PCIEFD_KCAN_CMD_SEQ_MASK, ++can->cmd_seq),
    ...
}

If a hardware status packet arrives (triggering
kvaser_pciefd_handle_status_packet() from the hardirq handler) while
kvaser_pciefd_bec_poll_timer() is polling from softirq, both invoke
kvaser_pciefd_send_kcan_cmd().

Since they increment can->cmd_seq without taking can->lock, could
concurrent read-modify-write operations lose increments or tear,
resulting in mismatched responses and state machine hangs?

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