Re: [PATCH v4 7/8] pmdomain: renesas: r8a78000: Add SCMI remapping support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] clk: double free of parent clock reference in error path
- [High] pmdomain: renesas: mutation of SCMI genpd and hardcoded index 0 in attach_dev
- [Medium] of: memory leak of scmi_clk_np reference
- [Medium] reset: memory leak of exclusive reset controls on error and unbind
- [Medium] pmdomain: renesas: out-of-bounds stack read in r8a78000_mdlc_attach_dev()
- [Low] cleanup: mixed goto and scope-based cleanups in attach_dev
--

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

pmdomain: renesas: r8a78000: Add SCMI remapping support

Extend the Module Controller driver for R-Car X5H with SCMI support, to
remap hardware power domain, module, and reset IDs to abstractions
offered by the SCMI firmware. This avoids putting SCMI IDs in DT and
supports fallback to non-SCMI defaults when needed.

> --- a/drivers/pmdomain/renesas/r8a78000-mdlc.c
> +++ b/drivers/pmdomain/renesas/r8a78000-mdlc.c
[ ... ]
> +static int r8a78000_mdlc_attach_dev(struct generic_pm_domain *domain,
> +				    struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct r8a78000_mdlc_priv *priv;
> +	struct clk *clk, *parent = NULL;
> +	struct of_phandle_args pd_spec;

[Severity: Medium]
Is pd_spec guaranteed to be fully initialized?

If the device tree node has a malformed #power-domain-cells < 2, could we
end up with uninitialized stack memory in pd_spec.args[1] that is later
used as a module ID?

> +	const struct mod_map *map;
> +	unsigned int id;
> +	int ret;
> +
> +	ret = of_parse_phandle_with_args(np, "power-domains",
> +					 "#power-domain-cells", 0, &pd_spec);

[Severity: High]
Is it intended to unconditionally parse index 0 here, even if the domain
being attached is at a higher index?

If a non-MDLC device uses multiple domains and the MDLC domain is not at
index 0, could this look up the wrong provider or silently fail to attach
the correct clocks?

> +	if (ret < 0)
> +		return ret;
> +
> +	scoped_guard(mutex, &r8a78000_mdlc_lock) {

[Severity: Low]
Does this mix of scope-based cleanup and goto violate the cleanup subsystem
guidelines?

This function uses scoped_guard() here, but also manually manages cleanup
using goto-based error handling labels later in the function (e.g.,
goto fail_put_parent). The guidelines suggest converting either all or none
of the resources in a given routine to avoid LIFO ordering bugs.

> +		hlist_for_each_entry(priv, &r8a78000_mdlc_list, link) {
> +			if (priv->np == pd_spec.np)
> +				break;
> +		}
> +	}
[ ... ]
> +	if (parent)
> +		ret = pm_clk_add_clk(dev, parent);
> +	if (!ret)
> +		ret = pm_clk_add_clk(dev, clk);
> +	if (ret)
> +		goto fail_destroy;
> +
> +	return 0;
> +
> +fail_destroy:
> +	pm_clk_destroy(dev);
> +fail_put_parent:
> +	clk_put(parent);

[Severity: High]
Will this result in a double free of the parent clock?

pm_clk_destroy() destroys all clocks attached to the device, which includes
cleanly removing the successfully added parent clock (and internally calling
clk_put() on it). Falling through to fail_put_parent seems to call
clk_put(parent) a second time.

> +fail_put_clk:
> +	clk_put(clk);
> +	return ret;
> +}
[ ... ]
> +static const struct power_map *fill_power_map(struct r8a78000_mdlc_priv *priv,
> +					      const struct power_map_in *map_in,
> +					      struct device_node *scmi_power_np)
> +{
[ ... ]
> +		genpd = of_genpd_get_from_provider(&scmi_spec);
> +		if (IS_ERR(genpd))
> +			return dev_err_cast_probe(dev, genpd,
> +					"Failed to get SCMI power domain %u\n",
> +					map[i].fw_id);
> +
> +		dev_dbg(dev, "SCMI power domain %u is %s\n", map[i].fw_id,
> +			genpd->name);
> +
> +		map[i].genpd = genpd;
> +
> +		/* Hook up clock domain support */
> +		genpd->attach_dev = r8a78000_mdlc_attach_dev;
> +		genpd->detach_dev = r8a78000_mdlc_detach_dev;
> +		/* Setting flags this late has no impact, but does not hurt */
> +		genpd->flags |= GENPD_FLAG_PM_CLK;
> +		genpd->dev_ops.stop = pm_clk_suspend;
> +		genpd->dev_ops.start = pm_clk_resume;

[Severity: High]
Is it safe to mutate the genpd struct of the SCMI provider directly?

Modifying genpd->attach_dev and genpd->dev_ops here changes the behavior
for all consumers of the shared SCMI domain. Could this cause
cross-subsystem state corruption or spurious warnings when non-MDLC devices
use this SCMI domain?

> +	}
> +
> +	return map;
> +}
> +
> +static const struct mod_map *fill_mod_map(struct r8a78000_mdlc_priv *priv,
> +					  const struct mod_map_in *map_in,
> +					  struct fwnode_handle *scmi_reset_fwnode)
> +{
[ ... ]
> +		scmi_spec.args[0] = map[i].fw_id;
> +
> +		rstc = reset_control_get_from_provider_exclusive(&scmi_spec);
> +		if (IS_ERR(rstc))
> +			return dev_err_cast_probe(dev, rstc,
> +					"Failed to get SCMI reset %u\n",
> +					map[i].fw_id);
> +
> +		map[i].rstc = rstc;

[Severity: Medium]
Does this error path leak the previously acquired exclusive reset controls?

If an allocation fails mid-loop, IS_ERR(rstc) evaluates to true and the
function immediately returns an error pointer, leaving no mechanism to free
map entries from earlier iterations.

Furthermore, the driver does not appear to iterate through this map and
release these controls during driver unbind or teardown.

> +	}
> +
> +	return map;
>  }
>  
>  static void r8a78000_mdlc_unlink(void *data)
>  {
>  	struct r8a78000_mdlc_priv *priv = data;
>  
>  	scoped_guard(mutex, &r8a78000_mdlc_lock) {
>  		hlist_del(&priv->link);
>  	}
>  }

[Severity: Medium]
Does this teardown sequence leak the priv->scmi_clk_np reference?

The reference was previously acquired using scmi_find_proto() with
SCMI_PROTOCOL_CLOCK, which returns an incremented reference count, but
of_node_put() does not appear to be called during unbind.

[ ... ]
>  static int r8a78000_mdlc_probe(struct platform_device *pdev)
>  {
> +	struct device_node *scmi_reset_np __free(device_node) = NULL;
> +	struct device_node *scmi_power_np __free(device_node) = NULL;
> +	struct device_node *scmi __free(device_node) = NULL;
[ ... ]
> +	scmi_clk_np = scmi_find_proto(scmi, SCMI_PROTOCOL_CLOCK);
> +	if (!scmi_clk_np) {
> +		dev_dbg(dev, "Cannot find SCMI clock management protocol\n");
> +		goto fallback;
> +	}
> +
> +	scmi_reset_np = scmi_find_proto(scmi, SCMI_PROTOCOL_RESET);
> +	if (!scmi_reset_np) {
> +		dev_dbg(dev, "Cannot find SCMI reset management protocol\n");
> +		goto fallback;
> +	}
> +
> +	priv->scmi_clk_np = scmi_clk_np;
> +	info = mdlc_data->fw_info;
> +
> +fallback:

[Severity: Medium]
If the jump to the fallback label occurs after successfully assigning
scmi_clk_np (e.g., when scmi_reset_np is missing), does this leak the
scmi_clk_np reference?

Since scmi_clk_np is not annotated with __free(device_node) like
scmi_power_np and scmi_reset_np, it requires explicit cleanup on error
paths.

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