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

[Biju Das]

Hi Wolfram Sang,

> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Sent: 25 June 2026 21:27
> Subject: Re: [PATCH] can: rcar_canfd: Change the initializing flow for clocks and resets
> 
> 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?

Thanks, Will send patches for fixing the pre-existing issues reported below.

Cheers,
Biju

> 
> > 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@xxxxxxxxxxx>
> >
> > 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
> > gpriv->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