Re: [PATCH v3 09/12] clk: renesas: r8a78000: Add SCMI remapping support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Leaked consumer `struct clk` instances and module references in `fill_clk_map()`.
- [High] Dangling `clk_hw` pointer dereference (Use-After-Free) due to missing device link to the SCMI provider.
- [Medium] Device node reference for `scmi_clk_np` is leaked on all exit paths in `r8a78000_cpg_probe`.
- [Low] Invalid `IS_ERR` check on `clk_hw` pointer resulting in unreachable error handling.
--

commit b6862fd9407ed1d4841735e0f963211d95acd921
Author: Geert Uytterhoeven <geert+renesas@glider.be>

clk: renesas: r8a78000: Add SCMI remapping support
    
Extend the Clock Pulse Generator driver for R-Car X5H with SCMI support,
to remap DT clock IDs to abstractions (clocks) offered by the SCMI
firmware implementation present when running on a system with a
supported SCMI firmware implementation.

> diff --git a/drivers/clk/renesas/r8a78000-cpg.c b/drivers/clk/renesas/r8a78000-cpg.c
> index 873d521ed3d6f..2273047315db5 100644
> --- a/drivers/clk/renesas/r8a78000-cpg.c
> +++ b/drivers/clk/renesas/r8a78000-cpg.c

[ ... ]

> @@ -120,11 +159,15 @@ static struct clk_hw **register_fixed_clks(struct device *dev)
>  }
>  
>  static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
> -					  const struct clk_map_in *map_in)
> +					  const struct clk_map_in *map_in,
> +					  struct device_node *scmi_clk_np)
>  {
> +	struct of_phandle_args scmi_spec;
>  	struct device *dev = priv->dev;
>  	struct clk_hw **fixed_hws;
>  	struct clk_map *map;
> +	struct clk_hw *hw;
> +	struct clk *clk;
>  	unsigned int i;
>  
>  	fixed_hws = register_fixed_clks(dev);
> @@ -143,7 +186,38 @@ static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
>  			break;
>  
>  		map[i].fw_id = map_in[i].fw_id;
> -		map[i].hw = fixed_hws[map[i].fw_id];
> +		if (map[i].fw_id >= FIXED_CLK_OFFSET) {
> +			enum fixed_clk idx = map[i].fw_id - FIXED_CLK_OFFSET;
> +
> +			map[i].hw = fixed_hws[idx];
> +			continue;
> +		}
> +
> +		scmi_spec.np = scmi_clk_np;
> +		scmi_spec.args_count = 1;
> +		scmi_spec.args[0] = map[i].fw_id;
> +
> +		clk = of_clk_get_from_provider(&scmi_spec);
> +		if (IS_ERR(clk))
> +			return dev_err_cast_probe(dev, clk,
> +				"Failed to get SCMI clock %u\n", map[i].fw_id);
> +
> +		hw = __clk_get_hw(clk);
> +		if (IS_ERR(hw))

[Severity: Low]
Is this error check reachable?  `__clk_get_hw()` extracts the hardware clock
from a valid `struct clk` and returns either a `struct clk_hw *` or `NULL`,
but it never returns an `ERR_PTR`.

> +			return dev_err_cast_probe(dev, hw,
> +				"Failed to get SCMI clock hw %u\n",
> +				map[i].fw_id);
> +
> +		if (!hw) {
> +			/* CLOCK_ATTRIBUTES is not supported */
> +			dev_warn(dev, "SCMI clock %u is NULL\n", map[i].fw_id);
> +			continue;
> +		}
> +
> +		dev_dbg(priv->dev, "SCMI clock %u is %s at %lu Hz\n",
> +			map[i].fw_id, clk_hw_get_name(hw), clk_hw_get_rate(hw));
> +
> +		map[i].hw = hw;

[Severity: High]
Does this code leak the consumer `struct clk` instances and module references?

`of_clk_get_from_provider()` dynamically allocates a consumer `struct clk` and
increments the SCMI provider's module reference.  This function extracts the
`clk_hw` pointer but abandons the `clk` pointer without calling `clk_put()`.

[Severity: High]
Could caching these raw `clk_hw` pointers without a device link lead to a
use-after-free?

If the SCMI provider is unbound via sysfs, it unregisters its clocks and frees
the `clk_hw` objects.  Because `renesas,scmi-firmware` is a custom property,
`fw_devlink` won't automatically establish unbind ordering.  This means a
subsequent clock request could cause the clock core to dereference a dangling
`clk_hw` pointer.

>  	}
>  
>  	devm_kfree(dev, fixed_hws);

[ ... ]

> @@ -152,20 +226,79 @@ static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv,
>  
>  static int r8a78000_cpg_probe(struct platform_device *pdev)
>  {
> +	struct device_node *scmi __free(device_node) = NULL;
> +	struct device_node *scmi_clk_np = NULL;

[Severity: Medium]
Does this code leak the device node reference?

`scmi_clk_np` is later assigned the result of `scmi_find_proto()`, which
acquires a node reference, but `scmi_clk_np` lacks the `__free(device_node)`
cleanup attribute and is never manually freed before returning on both success
and error paths.

>  	struct device *dev = &pdev->dev;
> +	const struct cpg_data *cpg_data;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788338320.git.geert+renesas@glider.be?part=9