Re: [PATCH 2/3] can: rcar_canfd: Fix double-cleanup and unclocked register access on init failure

[sashiko-bot@kernel.org]

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

New issues:
- [High] Use-after-free read on the error path of rcar_canfd_channel_probe().
- [High] The CAN network device is registered and exposed to userspace before its global and shared interrupts are requested, introducing a race condition.

Pre-existing issues:
- [High] Use-After-Free vulnerability in error and removal paths due to mismatched devm-managed interrupt lifetimes and dangling pointers.
- [High] Incorrect clearing of W0C (Write-0-To-Clear) hardware status registers can silently drop concurrent interrupts.
--

commit a601711f6b0c8a2b4d0262288f6d110cc98b8332
Author: Biju Das <biju.das.jz@bp.renesas.com>

can: rcar_canfd: Fix double-cleanup and unclocked register access on init failure

In rcar_canfd_probe(), if rcar_canfd_global_init() encounters an error,
it disables the clocks and asserts the resets before returning. However,
the probe function then jumps to the fail_mode label and unconditionally
calls rcar_canfd_global_deinit(), which writes to hardware registers and
causes a synchronous external abort. This commit makes the failure path
jump directly to fail_dev, skipping the redundant global_deinit() call.

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 5aafbab85da67..1f3dc4b354f87 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -2257,7 +2257,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  
>  	err = rcar_canfd_global_init(gpriv);
>  	if (err)
> -		goto fail_mode;
> +		goto fail_dev;
>  
>  	for_each_set_bit(ch, &gpriv->channels_mask, info->max_channels) {

[Severity: High]
Could this code expose the network interface to userspace before its interrupts
are fully set up?

The loop registers the channels via rcar_canfd_channel_probe(), which calls
register_candev(). However, the shared global interrupts (ch_irq, g_irq) are
not requested via devm_request_irq() until after the channel loop completes.

If a userspace process (e.g., systemd-networkd) brings the interface UP
immediately upon registration, could rcar_canfd_open() start the hardware and
enable interrupts before the OS has registered the IRQ handlers, leading to
unhandled interrupts?

>  		err = rcar_canfd_channel_probe(gpriv, ch, fcan_freq,
>  					       transceivers[ch]);
[ ... ]
> @@ -2275,7 +2275,7 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  fail_channel:
>  	for_each_set_bit(ch, &gpriv->channels_mask, info->max_channels)
>  		rcar_canfd_channel_remove(gpriv, ch);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free vulnerability
here due to mismatched interrupt lifetimes?

When rcar_canfd_channel_remove() frees the network device and its inline
private data priv via free_candev(priv->ndev), the devm_request_irq()
shared interrupt handlers remain active until devres cleanup runs after
probe() or remove() returns.

If a shared interrupt fires during this window, wouldn't the handler
(e.g., rcar_canfd_global_interrupt()) execute and unconditionally
dereference gpriv->ch[ch]->base, accessing the freed priv memory?

> -fail_mode:
> +
>  	rcar_canfd_global_deinit(gpriv, false);
>  fail_dev:
>  	return err;


I also noticed a few other issues in this file:

[Severity: High]
This is a pre-existing issue, but won't the driver incorrectly clear
W0C (Write-0-To-Clear) hardware status registers?

In functions like rcar_canfd_tx_done(), rcar_canfd_rx_poll(), and
rcar_canfd_global_error(), the driver reads the status register into sts
and writes back sts & ~FLAG:

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_tx_done() {
    ...
    rcar_canfd_write(priv->base, RCANFD_CFSTS(gpriv, ch, RCANFD_CFFIFO_IDX),
                     sts & ~RCANFD_CFSTS_CFTXIF);
    ...
}

Because this bitwise operation leaves 0s in bit positions that were
originally 0, won't writing 0 inadvertently clear any flag that was
asserted by the hardware after the register was initially read, leading to
silent loss of hardware interrupts and dropped packets?


[Severity: High]
Is there a use-after-free read in the error path of rcar_canfd_channel_probe()?

When device registration fails, the error path calls free_candev(ndev)
which frees the net_device and its inline private data priv:

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_channel_probe() {
    ...
fail:
	free_candev(ndev);
	gpriv->ch[priv->channel] = NULL;
	return err;
}

Immediately following the free, the code attempts to nullify the global
pointer array using priv->channel. Since priv has already been freed,
isn't this a use-after-free read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630135119.104774-1-biju.das.jz@bp.renesas.com?part=2