Re: [PATCH v4 5/7] clk: Use the generic OF phandle parsing in only one place

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Returning a dangling pointer to a device node's name after dropping the node's reference count.
- [High] Returning a dangling pointer to a clock core's name after dropping the clock reference.
- [High] Loss of the requested clock index when falling back to a parent node via the `clock-ranges` property.
--

--- Patch [5]: [PATCH v4 5/7] clk: Use the generic OF phandle parsing in only one place ---
commit ae5ff381e475c55a5d269ef4dce5a639c14cc437
Author: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>

clk: Use the generic OF phandle parsing in only one place

This commit replaces the use of of_parse_phandle_with_args() with
of_parse_clkspec() inside of_clk_get_parent_name(). It also modifies
of_parse_clkspec() to stop traversing up the device tree if a "clocks"
property is found.

> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 7d63d81ebc096..45f5d7a4ccc17 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -5219,6 +5219,8 @@ static int of_parse_clkspec(const struct device_node *np, int index,
>  		 * has a "clock-ranges" property, then we can try one of its
>  		 * clocks.
>  		 */
> +		if (of_property_present(np, "clocks"))
> +			break;
>  		np = np->parent;
>  		if (np && !of_property_present(np, "clock-ranges"))
>  			break;

[Severity: High]
This is a pre-existing issue, but this patch significantly exacerbates it.

When falling back to the parent node via the "clock-ranges" property, does
this discard the caller's requested clock index?

drivers/clk/clk.c:of_parse_clkspec() {
		...
		np = np->parent;
		if (np && !of_property_present(np, "clock-ranges"))
			break;
		index = 0;
	}

By hardcoding index to 0 before iterating to the parent, the requested clock
index is lost. The function incorrectly looks up index 0 on the parent node
regardless of what index the caller originally requested. This breaks
index-based clock lookups for devices relying on "clock-ranges" to inherit
clocks.

Since this patch switches of_clk_get_parent_name() to use of_parse_clkspec(),
when of_clk_parent_fill() requests parents 0, 1, 2, etc., will it now silently
receive identical copies of the parent's 0th clock name for all indices and
corrupt the hardware clock tree topology?

> @@ -5364,8 +5366,7 @@ const char *of_clk_get_parent_name(const struct device_node *np, int index)
>  	int count;
>  	struct clk *clk;
>  
> -	rc = of_parse_phandle_with_args(np, "clocks", "#clock-cells", index,
> -					&clkspec);
> +	rc = of_parse_clkspec(np, index, NULL, &clkspec);
>  	if (rc)
>  		return NULL;
>  

[Severity: High]
This isn't a bug introduced by this patch, but looking further down in
of_clk_get_parent_name(), does this return a dangling pointer if the device
tree node is dynamically removed?

drivers/clk/clk.c:of_clk_get_parent_name() {
		...
		if (IS_ERR(clk)) {
			if (clkspec.args_count == 0)
				clk_name = clkspec.np->name;
			else
				clk_name = NULL;
		} else {
		...
	of_node_put(clkspec.np);
	return clk_name;
}

The string pointer clk_name is retrieved from the device node, but the
reference to the node is immediately dropped via of_node_put(clkspec.np). If
the node and its name string are freed, for example via DT overlay removal,
any subsequent access to the child's cached parent name by the clock framework
would lead to a Use-After-Free.

[Severity: High]
This is another pre-existing issue in the same function, but does this also
return a dangling pointer if the parent clock provider is dynamically
unregistered?

drivers/clk/clk.c:of_clk_get_parent_name() {
		...
		} else {
			clk_name = __clk_get_name(clk);
			clk_put(clk);
		}
	}

	of_node_put(clkspec.np);
	return clk_name;
}

The clock name is retrieved via __clk_get_name(clk), but the consumer
reference is immediately dropped via clk_put(clk). If the parent clock
provider unregisters (e.g., on driver unbind or module unload), the parent's
clk_core structure and its name string are freed. This could trigger a
Use-After-Free on any subsequent access by the child clock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-schneider-v7-2-rc1-eip201-upstream-v4-0-751547e160e5@bootlin.com?part=5