Re: [PATCH v3 4/8] can: rcar_canfd: Allow the CAN FD clock to be sourced from fck

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

Pre-existing issues:
- [High] The driver ignores `-EPROBE_DEFER` when acquiring the optional `can_clk`, leading to an incorrect clock source fallback or spurious probe failures.
- [High] Double free of `ndev` and UAF via `unregister_candev` if `register_candev` fails during channel probe.
- [High] Unbalanced clock disable and reset assert when `rcar_canfd_global_init` fails.
--

--- Patch [4]: [PATCH v3 4/8] can: rcar_canfd: Allow the CAN FD clock to be sourced from fck ---
commit a52a5e318c0ad2d299e214595e95da5df928f319
Author: Claudiu Beznea <[email protected]>

can: rcar_canfd: Allow the CAN FD clock to be sourced from fck

This commit adjusts the clock handling in the RZ/G3S CAN FD driver to allow
the clock to be sourced from the peripheral clock instead of an external
clock. It also updates existing hardware info structures to support this.

Link: https://lore.kernel.org/all/[email protected]

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index b40ae5998c1da..2e84dc5dfa34d 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
[ ... ]
> @@ -2190,13 +2196,20 @@ static int rcar_canfd_probe(struct platform_device *pdev)
>  	 */
>  	gpriv->can_clk = devm_clk_get(dev, "can_clk");
>  	if (IS_ERR(gpriv->can_clk) || (clk_get_rate(gpriv->can_clk) == 0)) {
> -		gpriv->can_clk = devm_clk_get(dev, "canfd");
> -		if (IS_ERR(gpriv->can_clk))
> -			return dev_err_probe(dev, PTR_ERR(gpriv->can_clk),
> -					     "cannot get canfd clock\n");
> +		if (info->fcan_pclk) {

[Severity: High]
This is a pre-existing issue, but the driver appears to ignore -EPROBE_DEFER
when acquiring the optional can_clk.

If can_clk is defined in the device tree but its provider has not yet probed,
devm_clk_get() returns -EPROBE_DEFER. Because the driver checks IS_ERR() and
blindly coalesces all errors into the fallback path, platforms with fcan_pclk
will silently fall back to the peripheral clock instead of deferring the probe.

Could this cause the CAN controller to run on the wrong clock, resulting in
invalid CAN baud rates? Should devm_clk_get_optional() be used here to
correctly propagate -EPROBE_DEFER?

> +			fcan_freq = clk_get_rate(gpriv->clkp);
> +			gpriv->can_clk = NULL;
> +		} else {
> +			gpriv->can_clk = devm_clk_get(dev, "canfd");
> +			if (IS_ERR(gpriv->can_clk))
> +				return dev_err_probe(dev, PTR_ERR(gpriv->can_clk),
> +						     "cannot get canfd clock\n");
> +
> +			fcan_freq = clk_get_rate(gpriv->can_clk);
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a double free
and use-after-free risk during channel probe failure.

In rcar_canfd_channel_probe(), the channel pointer is assigned before calling
register_candev():

	gpriv->ch[priv->channel] = priv;
	err = register_candev(ndev);
	if (err) {
		...
		goto fail_candev;
	}

If register_candev() fails, the error path frees ndev (which embeds priv) via
free_candev(ndev), but leaves the dangling pointer in gpriv->ch.

When this error propagates back to rcar_canfd_probe(), the cleanup loop calls
rcar_canfd_channel_remove() for all channels:

	struct rcar_canfd_channel *priv = gpriv->ch[ch];
	if (priv) {
		unregister_candev(priv->ndev);
		netif_napi_del(&priv->napi);
		free_candev(priv->ndev);
	}

Will this dereference the dangling pointer, incorrectly call unregister_candev()
on invalid memory, and double-free ndev?

[Severity: High]
This is a pre-existing issue, but there seems to be an unbalanced clock disable
and reset assert when rcar_canfd_global_init() fails.

Inside rcar_canfd_global_init(), if an initialization step fails, the error
path correctly cleans up:

fail_ram_clk:
	clk_disable_unprepare(gpriv->clk_ram);
fail_clk:
	clk_disable_unprepare(gpriv->clkp);
fail_reset2:
	reset_control_assert(gpriv->rstc2);
fail_reset1:
	reset_control_assert(gpriv->rstc1);
	return err;

However, the error then propagates back to rcar_canfd_probe(), which jumps
to fail_mode and blindly calls rcar_canfd_global_deinit():

fail_mode:
	rcar_canfd_global_deinit(gpriv, false);

Since rcar_canfd_global_deinit() disables the exact same clocks and asserts
the exact same resets again, could this result in an unbalanced clock disable
(enable count underflow) and trigger kernel warnings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4