Re: [PATCH v3 10/12] pmdomain: renesas: r8a78000: Add SCMI remapping support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unsafe lockless modification of a globally shared SCMI generic_pm_domain structure breaks SCMI consumers.
- [High] Use-After-Free of priv structure in r8a78000_mdlc_attach_dev() due to lockless access.
- [Medium] Memory leak of Open Firmware (OF) device nodes acquired via scmi_find_proto().
- [Medium] Resource leak of reset_control structures acquired via reset_control_get_from_provider_exclusive().
- [Low] Mixing goto-based error cleanup and scope-based cleanups in r8a78000_mdlc_attach_dev.
--

commit 61539c706c75d056855ecfa70e1aa4d445ba46bf
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
(power domains, clocks, and resets) offered by the SCMI firmware.

> diff --git a/drivers/pmdomain/renesas/r8a78000-mdlc.c b/drivers/pmdomain/renesas/r8a78000-mdlc.c
> index 2668fc2ac9cbd..0238bf8c1e385 100644
> --- 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 of_phandle_args pd_spec, scmi_spec;
>  	struct device_node *np = dev->of_node;
>  	struct r8a78000_mdlc_priv *priv;

[ ... ]

> +	scoped_guard(mutex, &r8a78000_mdlc_lock) {
> +		hlist_for_each_entry(priv, &r8a78000_mdlc_list, link) {
> +			if (priv->np == pd_spec.np)
> +				break;
> +		}
> +	}
> +
> +	if (!priv) {
> +		dev_err(dev, "%s: MDLC %pOF not found\n", __func__, pd_spec.np);
> +		of_node_put(pd_spec.np);
> +		return -ENODEV;
> +	}
> +
> +	id = pd_spec.args[1];
> +	of_node_put(pd_spec.np);
> +
> +	map = mod_map_find(priv->mod_map, id);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
After scoped_guard() releases the lock, if the MDLC device is concurrently
unbound via sysfs, could r8a78000_mdlc_unlink() remove and free priv before
it is accessed here locklessly, resulting in a use-after-free?

[ ... ]

> +	ret = pm_clk_create(dev);
> +	if (ret)
> +		goto fail_put;
> +
> +	ret = pm_clk_add_clk(dev, clk);
> +	if (ret)
> +		goto fail_destroy;
> +
>  	return 0;
> +
> +fail_destroy:
> +	pm_clk_destroy(dev);
> +fail_put:
> +	clk_put(clk);
> +	return ret;
> +}

[Severity: Low]
Since this function uses scoped_guard(), does mixing it with goto-based error
handling conflict with the cleanup guidelines, which recommend converting all
resources to scope-based cleanup or none of them within a given routine?

[ ... ]

> +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)
> +{
> +	struct of_phandle_args scmi_spec;
> +	struct generic_pm_domain *genpd;

[ ... ]

> +		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]
Does overwriting the callbacks on the external genpd structure owned by the
SCMI provider break native SCMI clients? If a client bypasses MDLC and
attaches directly to the SCMI domain, the core will invoke
r8a78000_mdlc_attach_dev(), which expects an MDLC node and returns -ENODEV,
potentially denying access.

[ ... ]

> +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)
> +{
> +	struct fwnode_reference_args scmi_spec;
> +	struct device *dev = priv->dev;
> +	struct reset_control *rstc;

[ ... ]

> +		scmi_spec.fwnode = scmi_reset_fwnode;
> +		scmi_spec.nargs = 1;
> +		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 reset_control_get_from_provider_exclusive() return an unmanaged
reference that must be explicitly freed? If so, will returning an error on a
subsequent loop iteration leak the references acquired in earlier iterations?
There also appears to be no reset_control_put() cleanup during driver unbind.

[ ... ]

>  static int r8a78000_mdlc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> +	struct device_node *scmi __free(device_node) = NULL;
> +	struct device_node *scmi_power_np = NULL;
> +	const struct mdlc_data *mdlc_data;
> +	struct device_node *scmi_reset_np;
> +	struct device_node *scmi_clk_np;

[ ... ]

> +	scmi_power_np = scmi_find_proto(scmi, SCMI_PROTOCOL_POWER);
> +	if (!scmi_power_np) {
> +		dev_dbg(dev,
> +			"Cannot find SCMI power domain management protocol\n");
> +		goto fallback;
> +	}
> +
> +	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;
> +	}
> +
> +	ret = scmi_get_base_info(scmi, &version);
> +	if (ret == -EPROBE_DEFER)
> +		return dev_err_probe(dev, ret, "SCMI provider not ready\n");

[Severity: Medium]
Since scmi_find_proto() returns device nodes with an elevated reference count,
do these references leak on this -EPROBE_DEFER early return? Additionally,
they do not appear to be released with of_node_put() on the success path
either.

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