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

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 4774 bytes --]

Hi Biju,

Sashiko found issues with your patch. I curated the list and left those
which I see as reasonable. I know that being pointed to pre-existing
issues is annoying, but maybe you have interest to look at these issues?

Thank you and happy hacking,

   Wolfram

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]