Re: [PATCH v3 4/6] pmdomain: renesas: Add R-Car X5H MDLC driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The attach_dev callback hardcodes index 0 when parsing the 'power-domains' property, breaking support for devices with multiple power domains.
- [High] Unprotected access to the provider's private data (`priv`) outside `r8a78000_mdlc_lock` in `r8a78000_mdlc_attach_dev` leads to a potential Use-After-Free.
--

--- Patch [4]: [PATCH v3 4/6] pmdomain: renesas: Add R-Car X5H MDLC driver ---
commit 72c0935c5567aec22a55250554e605471b44c678
Author: Geert Uytterhoeven <[email protected]>

pmdomain: renesas: Add R-Car X5H MDLC driver

This commit adds a minimal Module Controller driver for the R-Car X5H SoC. It
currently supports always-on power domains along with dummy module clocks and
resets for the serial console.

> diff --git a/drivers/pmdomain/renesas/r8a78000-mdlc.c b/drivers/pmdomain/renesas/r8a78000-mdlc.c
> new file mode 100644
> index 0000000000000..5c8acead7cff9
> --- /dev/null
> +++ 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 of_phandle_args pd_spec;
> +	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]
Does this hardcode the index to 0 when parsing the power-domains property?

If a consumer device specifies multiple power domains in its device tree
and the MDLC domain is not the first one, the genpd framework calling
r8a78000_mdlc_attach_dev() will still parse index 0. Would this result
in an unmatched node pointer, causing attach_dev to erroneously reject
the attachment with -ENODEV?

Could the module ID validation perhaps be performed inside
r8a78000_genpd_xlate() instead, where the exact spec is readily available
without needing to parse the device tree again?

> +	if (ret < 0)
> +		return ret;
> +
> +	if (pd_spec.args_count != 2) {
> +		of_node_put(pd_spec.np);
> +		return -EINVAL;
> +	}
> +
> +	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]
Can this lead to a use-after-free on the priv pointer?

The scoped_guard block above locates the priv pointer but releases
r8a78000_mdlc_lock as soon as the block ends. 

If the MDLC provider probe fails later (e.g. in devm_reset_controller_register)
or is unbound, devres unwinds and calls r8a78000_mdlc_unlink() to remove priv
from the list, then frees it.

If a consumer attaches to the singleton domain and races with this unbind,
could it dereference priv->mod_map here after priv has already been freed?

> +	if (!map) {
> +		dev_err(dev, "Unknown module 0x%x\n", id);
> +		return -ENOENT;
> +	}
> +
> +	dev_dbg(dev, "Ignoring HW module 0x%x\n", id);
> +	return 0;
> +}

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