Re: [PATCH RFC v2 2/9] clk: renesas: r9a09g077: Add SYSC regmap support

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

New issues:
- [High] Registering a devres-managed regmap with the global syscon_list causes a Use-After-Free upon probe failure.
- [High] Missing of_node_get() before registering the child node with syscon risks a Use-After-Free of the device node.
- [Medium] The commit message contradicts the implementation regarding which device node the regmap is registered against.

Pre-existing issues:
- [High] Failing to explicitly remove the PM domain provider on probe error leaves a dangling provider pointing to freed memory.
--

commit db570cc0b618206556f7cc6978347f7824aa7f3f
Author: Lad Prabhakar <[email protected]>

clk: renesas: r9a09g077: Add SYSC regmap support

Add System Controller (SYSC) regmap support for the RZ/T2H SoC.

The RZ/T2H SoC features two System Controller (SYSC) blocks that handle
low-power management and access control. These blocks are defined as child
nodes within the Clock Pulse Generator (CPG) address space in the device
tree. Implement support for creating regmaps for these SYSC blocks.

> Register the regmap using of_syscon_register_regmap() against the CPG
> device node so that consumers can access SYSC registers via the existing
> syscon phandle.

[Severity: Medium]
The commit message states the regmap is registered against the CPG device
node, but the code actually registers it against the "system-controller"
child node. Could the commit message be updated to match the implementation?

> Add a sysc_init() callback to the CPG/MSSR core to allow SoC-specific
> initialization of the SYSC regmap.
> 
> Signed-off-by: Lad Prabhakar <[email protected]>

> diff --git a/drivers/clk/renesas/r9a09g077-cpg.c b/drivers/clk/renesas/r9a09g077-cpg.c
> index 5640c2035e5a4..33aec2f272df4 100644
> --- a/drivers/clk/renesas/r9a09g077-cpg.c
> +++ b/drivers/clk/renesas/r9a09g077-cpg.c
[ ... ]
> +static int r9a09g077_sysc_init(struct device *dev)
> +{
> +	struct device_node *child, *np = dev->of_node;
> +
> +	for_each_child_of_node_scoped(np, child) {
> +		struct regmap_config *regmap_cfg __free(kfree) = kzalloc_obj(*regmap_cfg);
> +		struct r9a09g077_sysc_reg *sysc_reg;
> +		struct regmap *regmap;
> +		void __iomem *base;
> +		char *name;
> +		u32 index;
> +		int ret;
[ ... ]
> +		regmap = devm_regmap_init(dev, &rzt2h_sys_regmap_bus, sysc_reg, regmap_cfg);
> +		if (IS_ERR(regmap))
> +			return PTR_ERR(regmap);
> +
> +		ret = of_syscon_register_regmap(child, regmap);

[Severity: High]
If the probe fails on a subsequent step (for instance, during the next loop
iteration), devres will automatically free the regmap since it was allocated
using devm_regmap_init().

However, of_syscon_register_regmap() saves the regmap pointer in a permanent
global list (syscon_list), which has no unregistration mechanism. Does this
leave a dangling pointer in the global list, leading to a use-after-free on
subsequent syscon lookups?

[Severity: High]
Additionally, the child node reference is borrowed by
for_each_child_of_node_scoped() and will be automatically dropped at the end
of the loop iteration.

Since of_syscon_register_regmap() internally saves the raw device_node pointer
into its global list without incrementing its reference count, could this
result in a use-after-free of the device node if the device tree node is ever
dynamically removed? Should we call of_node_get() before registering it?

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c
> index 4ed056b18d31f..a46ea82ed85e0 100644
> --- a/drivers/clk/renesas/renesas-cpg-mssr.c
> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c
> @@ -1415,6 +1415,9 @@ static int __init cpg_mssr_probe(struct platform_device *pdev)
>  
>  	error = cpg_mssr_reset_controller_register(priv);
>  
> +	if (!error && info->sysc_init)
> +		error = info->sysc_init(priv->dev);
> +

[Severity: High]
This is a pre-existing issue, but this new error path exposes it more easily.

Earlier in cpg_mssr_probe(), cpg_mssr_add_clk_domain() adds an OF genpd
provider. While devres is set up to call pm_genpd_remove(), that function
refuses to remove domains that still have active OF providers (returning
-EBUSY).

If probe fails here in info->sysc_init(), devres will proceed to free the
cpg_mssr_clk_domain structure. Because the domain wasn't actually removed
from the global lists, the active OF provider remains pointing to freed memory.

Does the driver need to explicitly call of_genpd_del_provider() on error paths
to prevent this use-after-free?

>  reserve_exit:
>  	cpg_mssr_reserved_exit(priv);
>

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