Re: [PATCH net-next 17/37] can: rcar_canfd: Add support for Renesas RZ/G3S

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] On renesas,r9a08g045-canfd (RZ/G3S), rcar_canfd_probe() reads the
  "renesas,no-can-fd" and "renesas,fd-only" DT properties without…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a use-
  after-free and double free in the probe error path, now also reachable…
- [High] This problem wasn't introduced by this patch, but it is a use-
  after-free in IRQ context during driver removal, also reachable on the…

--

can: rcar_canfd: Add support for Renesas RZ/G3S

This adds r9a08g045_hw_info for the RZ/G3S CAN-FD controller: two
channels, 16 AFL pages, a CAN FD clock derived from pclk and a fixed
CAN-FD mode. It also adds the "renesas,r9a08g045-canfd" compatible to the
OF match table.

Link: https://patch.msgid.link/20260902141544.565763-6-claudiu.beznea+renesas@tuxon.dev

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index ac5d5959d8e4..74c3c3f20991 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -661,6 +661,26 @@ static const struct rcar_canfd_hw_info rzg2l_hw_info = {
>  	.fixed_canfd = 0,
>  };
>  
> +static const struct rcar_canfd_hw_info r9a08g045_hw_info = {

[ ... ]

> +	.external_clk = 1,
> +	.fcan_pclk = 1,
> +	.fixed_canfd = 1,
> +};

[Severity: Low]
This is the first hw_info with fixed_canfd set. That makes the early
continue in rcar_canfd_reset_controller() reachable:

	/* Set the controller into appropriate mode */
	if (gpriv->info->fixed_canfd)
		continue;

As a result CLOE/FDOE are never programmed, and the hardware stays in
CAN-FD mode.

rcar_canfd_probe() still reads the mode properties for every compatible
and does not check info->fixed_canfd:

	if (of_property_read_bool(dev->of_node, "renesas,no-can-fd"))
		fdmode = false;			/* Classical CAN only mode */
	...
	if (of_property_read_bool(dev->of_node, "renesas,fd-only"))
		gpriv->fd_only_mode = true; /* FD-Only mode for CAN-FD */

Suppose renesas,no-can-fd were present on an RZ/G3S node. Then
gpriv->fdmode is false, and rcar_canfd_channel_probe() sets the channel
up as classical CAN: no CAN_CTRLMODE_FD, no data bittiming, and RFFDF
ignored on rx. The controller would still be in CAN-FD mode. Would the
software mode and the hardware mode then disagree?

With renesas,fd-only, the only effect would be that the "global
operational state" message prints "fd-only". That doesn't match the
hardware either.

The binding added earlier in this series sets renesas,no-can-fd and
renesas,fd-only to false for renesas,r9a08g045-canfd. A DT that passes
schema validation therefore can't reach this path; only a DT that breaks
the binding can.

Would it still make sense to ignore these properties, or warn about
them, when info->fixed_canfd is set? The RZ/G3L compatible added later
in the series reuses r9a08g045_hw_info and has the same behaviour.

[ ... ]

> @@ -2383,6 +2403,7 @@ static DEFINE_SIMPLE_DEV_PM_OPS(rcar_canfd_pm_ops, rcar_canfd_suspend,
>  
>  static const __maybe_unused struct of_device_id rcar_canfd_of_table[] = {
>  	{ .compatible = "renesas,r8a779a0-canfd", .data = &rcar_gen4_hw_info },
> +	{ .compatible = "renesas,r9a08g045-canfd", .data = &r9a08g045_hw_info },

[Severity: High]
This isn't a bug introduced by this patch, but RZ/G3S can now hit a
use-after-free and double free in the probe error path.

rcar_canfd_channel_probe() publishes the channel before registering it,
and does not clear the slot if registration fails:

	gpriv->ch[priv->channel] = priv;
	err = register_candev(ndev);
	if (err) {
		...
		goto fail_candev;
	}
	...
fail_candev:
	netif_napi_del(&priv->napi);
fail:
	free_candev(ndev);

priv lives in netdev_priv(ndev), so after this gpriv->ch[ch] points to
freed memory. rcar_canfd_probe() then unwinds every channel in
channels_mask, including the one that just failed:

fail_channel:
	for_each_set_bit(ch, &gpriv->channels_mask, info->max_channels)
		rcar_canfd_channel_remove(gpriv, ch);

The stale pointer passes the NULL check in rcar_canfd_channel_remove():

	struct rcar_canfd_channel *priv = gpriv->ch[ch];

	if (priv) {
		unregister_candev(priv->ndev);
		netif_napi_del(&priv->napi);
		free_candev(priv->ndev);
	}

Suppose register_candev() fails, for example with -ENOMEM from
register_netdevice() or a NETDEV_REGISTER notifier veto. Doesn't this
path read priv->ndev from freed memory, call unregister_candev() on a
netdev that was never registered, and then call free_candev() on it a
second time?

The assignment comes from commit c5048a7b2c23 ("can: rcar_canfd:
rcar_canfd_channel_probe(): register the CAN device when fully ready").
The error path dates back to commit dd3bd23eb438 ("can: rcar_canfd: Add
Renesas R-Car CAN FD driver"). Nothing later in the series changes it.

Two possible fixes: clear gpriv->ch[priv->channel] on the fail_candev
path, or assign it only after register_candev() succeeds (after checking
that no IRQ handler needs it earlier).

[Severity: High]
This is also a pre-existing issue and not introduced by this patch. Is
there a use-after-free in IRQ context during rcar_canfd_remove() that
RZ/G3S now inherits?

	for_each_set_bit(ch, &gpriv->channels_mask, gpriv->info->max_channels) {
		rcar_canfd_disable_channel_interrupts(gpriv->ch[ch]);
		rcar_canfd_channel_remove(gpriv, ch);
	}

	rcar_canfd_global_deinit(gpriv, true);

rcar_canfd_disable_channel_interrupts() only clears the CCTR error
interrupt enables. It does not touch the RX FIFO RFIE bit.

rcar_canfd_channel_remove() then frees the channel but leaves
gpriv->ch[ch] and gpriv->channels_mask unchanged.

Global interrupts are only disabled afterwards, in
rcar_canfd_global_deinit(). The global IRQ handlers were requested with
devm_request_irq() and dev_id = gpriv, so they stay installed until after
remove() returns.

Consider the case where channel 0 has been freed but channel 1 is still
open and receiving. An RX FIFO interrupt for channel 1 runs
rcar_canfd_global_receive_fifo_interrupt() (g_recc on RZ/G3S and RZ/G2L)
or rcar_canfd_global_interrupt() (Gen3/Gen4). Both walk all of
channels_mask, starting with channel 0:

rcar_canfd_global_receive_fifo_interrupt()
  rcar_canfd_handle_global_receive(gpriv, 0)
    priv = gpriv->ch[0];   /* already freed */
    sts = rcar_canfd_read(priv->base, RCANFD_RFSTS(gpriv, ridx));

Could this dereference the freed priv and perform MMIO through a base
address loaded from freed memory? The global error path also uses
priv->ndev of the freed channel and can call rcar_canfd_global_error()
on it.

Nothing later in the series addresses this. Either of these would close
the window: disable all global and per-channel interrupt sources and
synchronize the IRQs before freeing any channel, or clear gpriv->ch[ch]
and its channels_mask bit before the free.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015015336.816543-2-mkl%40pengutronix.de