Re: [PATCH] can: rcar_canfd: Change the initializing flow for clocks and resets

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Double-cleanup and unclocked register access on initialization failure in `rcar_canfd_probe()`.
- [High] Premature registration of global interrupt handlers causes NULL pointer dereference and unclocked register access.
- [High] Dangling pointer in the global state array leads to a double free on channel probe failure.
- [High] Mismatch between `devm_request_irq` and manual memory freeing causes use-after-free.
--

commit 6b6a83169ac0a7297a4dc2c78e16dd98f4f2ed0f
Author: Tu Nguyen <tu.nguyen.xg@renesas.com>

can: rcar_canfd: Change the initializing flow for clocks and resets

This patch updates the initialization order for clocks and resets in the
rcar_canfd driver to match the hardware manual for RZ/G2L and RZ/G3E.
It ensures clocks are enabled before deasserting resets, and updates the
teardown path to mirror the new sequence.

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index eaf8cac780387..fcc37b73ed436 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
[ ... ]
> @@ -2090,10 +2090,10 @@ static void rcar_canfd_global_deinit(struct rcar_canfd_global *gpriv, bool full)
>  		rcar_canfd_set_bit(gpriv->base, RCANFD_GCTR, RCANFD_GCTR_GSLPR);
>  	}
>  
> -	clk_disable_unprepare(gpriv->clk_ram);
> -	clk_disable_unprepare(gpriv->clkp);
>  	reset_control_assert(gpriv->rstc2);
>  	reset_control_assert(gpriv->rstc1);
> +	clk_disable_unprepare(gpriv->clk_ram);
> +	clk_disable_unprepare(gpriv->clkp);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing 
potential double-cleanup and unclocked register access on initialization 
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(). 

The deinit function calls rcar_canfd_disable_global_interrupts(), which 
writes to the hardware registers. Since the peripheral clock has already 
been disabled by the failed init, does this write trigger a synchronous 
external abort or bus fault?

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with 
interrupt registration order.

In rcar_canfd_probe(), global interrupts are requested via 
devm_request_irq() before rcar_canfd_global_init() enables the clocks and 
before the per-channel structures in gpriv->ch[] are allocated. 

If the interrupt is shared and fires immediately, the handler 
rcar_canfd_global_err_interrupt() will invoke rcar_canfd_handle_global_err().
This function dereferences priv = gpriv->ch[ch], which is currently NULL.
Could this lead to a NULL pointer dereference, as well as an unclocked 
hardware read?

[Severity: High]
This is a pre-existing issue, but there appears to be a dangling pointer 
risk on channel probe failure.

In rcar_canfd_channel_probe(), the channel pointer is saved globally via 
gpriv->ch[priv->channel] = priv. If register_candev() fails shortly after, 
the error path uses free_candev(ndev) to release the memory but leaves the 
pointer in the global array. 

During teardown, rcar_canfd_probe() calls rcar_canfd_channel_remove() for 
all channels in the mask. Since the dangling pointer still evaluates to true, 
will this result in a use-after-free and double free of the memory?

[Severity: High]
This isn't a bug introduced by this patch, but there might be a 
use-after-free vulnerability with the device-managed IRQs.

In rcar_canfd_channel_probe(), the per-channel interrupt is requested 
using devm_request_irq() with priv as the callback data. However, 
rcar_canfd_channel_remove() manually frees the channel structure using 
free_candev(priv->ndev). 

Because the devm-managed IRQ is not unregistered until the driver probe or 
remove function completely returns, could an interrupt firing in this 
teardown window access the freed priv structure?

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