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

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

New issues:
- [High] Unhandled ERR_PTR dereference leading to kernel panic in clock API.

Pre-existing issues:
- [High] Use-After-Free and Double Free on probe error path.
--

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

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

Adjust the existing canfd clock handling code to support the RZ/G3S CAN.
When fck is used, it is divided internally by the CAN FD controller.

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..0283620ebcf8d 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
[ ... ]
> @@ -2190,13 +2196,19 @@ 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) {
> +			fcan_freq = clk_get_rate(gpriv->clkp);
> +		} else {

[Severity: High]
If info->fcan_pclk is true and can_clk is not present, gpriv->can_clk
retains an ERR_PTR value here.

When the interface is brought up later, rcar_canfd_open() calls
clk_prepare_enable(gpriv->can_clk):

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_open() {
    ...
	/* Peripheral clock is already enabled in probe */
	err = clk_prepare_enable(gpriv->can_clk);
	if (err) {
    ...
}

Because clk_prepare_enable() checks for NULL but not for IS_ERR(), will
this dereference the error pointer and cause a kernel panic? Should
gpriv->can_clk be explicitly set to NULL when using the fcan_pclk fallback?

> +			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);
> +		}
>  
>  		/* CANFD clock may be further divided within the IP */
> -		fcan_freq = clk_get_rate(gpriv->can_clk) / info->postdiv;
> +		fcan_freq /= info->postdiv;
>  	} else {
>  		fcan_freq = clk_get_rate(gpriv->can_clk);
>  		gpriv->extclk = gpriv->info->external_clk;

[Severity: High]
This is a pre-existing issue, but there is a use-after-free and double
free on the probe error path.

In rcar_canfd_channel_probe():

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_channel_probe() {
    ...
	gpriv->ch[priv->channel] = priv;
	err = register_candev(ndev);
	if (err) {
		dev_err(dev, "register_candev() failed: %pe\n", ERR_PTR(err));
		goto fail_candev;
	}
    ...
fail_candev:
	netif_napi_del(&priv->napi);
fail:
	free_candev(ndev);
	return err;
}

If register_candev() fails, ndev (which contains priv) is freed via
free_candev(), but the global pointer gpriv->ch[priv->channel] is not
cleared.

Then in the parent rcar_canfd_probe(), the error handler cleans up:

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_probe() {
    ...
fail_channel:
	for_each_set_bit(ch, &gpriv->channels_mask, info->max_channels)
		rcar_canfd_channel_remove(gpriv, ch);
    ...
}

And rcar_canfd_channel_remove() bypasses the NULL check:

drivers/net/can/rcar/rcar_canfd.c: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);
	}
}

Will this cause unregister_candev() to be called on an already freed device,
followed by a second free_candev()?

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