Re: [PATCH v2 6/7] clk: Add support for clock nexus dt bindings

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Stack buffer overflow in `of_parse_phandle_with_args_map` when `#<list>-cells` exceeds `MAX_PHANDLE_ARGS`.
- [High] Infinite loop in `of_parse_phandle_with_args_map` due to lack of cycle detection.
- [High] Unconditional dereference of optional `out_args` pointer in `of_parse_phandle_with_args_map`.
--

--- Patch [6]: [PATCH v2 6/7] clk: Add support for clock nexus dt bindings ---
commit 2eb2cbfe08397c2ed1b0b80363cc91255e359912
Author: Miquel Raynal (Schneider Electric) <miquel.raynal@bootlin.com>

clk: Add support for clock nexus dt bindings

This commit introduces support for nexus nodes in the clock subsystem. It
maps inner clocks to inlet clocks by transitioning from
of_parse_phandle_with_args() to of_parse_phandle_with_args_map().

> diff --git a/drivers/clk/clk-conf.c b/drivers/clk/clk-conf.c
> index 303a0bb26e54a..5380d43b56a42 100644
> --- a/drivers/clk/clk-conf.c
> +++ b/drivers/clk/clk-conf.c
> @@ -25,8 +25,8 @@ static int __set_clk_parents(struct device_node *node, bool clk_supplier)
>  		       node);
>  
>  	for (index = 0; index < num_parents; index++) {
> -		rc = of_parse_phandle_with_args(node, "assigned-clock-parents",
> -					"#clock-cells",	index, &clkspec);
> +		rc = of_parse_phandle_with_args_map(node, "assigned-clock-parents",
> +						    "clock", index, &clkspec);

[Severity: Critical]
This is a pre-existing issue, but by adopting of_parse_phandle_with_args_map(),
does this expose the clock subsystem to a stack buffer overflow?

When a device tree node specifies a cell count greater than MAX_PHANDLE_ARGS,
__of_parse_phandle_with_args() correctly clamps the count.

However, of_parse_phandle_with_args_map() in drivers/of/base.c re-reads the
unclamped cell count into list_size:

drivers/of/base.c:of_parse_phandle_with_args_map() {
    ...
    ret = of_property_read_u32(cur, cells_name, &list_size);
    if (ret < 0)
        goto put;

    /* Precalculate the match array - this simplifies match loop */
    for (i = 0; i < list_size; i++)
        initial_match_array[i] = cpu_to_be32(out_args->args[i]);
    ...
}

Could this overflow initial_match_array[] if list_size exceeds
MAX_PHANDLE_ARGS?

[Severity: High]
This is a pre-existing issue, but does switching to this API also expose the
subsystem to an infinite loop if a device tree overlay introduces a cycle?

In drivers/of/base.c:of_parse_phandle_with_args_map(), the map resolution loop
continuously transitions to the next node:

drivers/of/base.c:of_parse_phandle_with_args_map() {
    ...
    while (cur) {
        /* Get the <list>-map property */
        map = of_get_property(cur, map_name, &map_len);
    ...
}

Does this while loop lack cycle detection or a maximum depth bound, potentially
allowing malformed input to hang the CPU?

[Severity: High]
This is a pre-existing issue, but does of_parse_phandle_with_args_map()
unconditionally dereference out_args?

While this patch passes a valid pointer, the API is documented as accepting
an optional out_args pointer. If a future caller passes NULL to validate a
mapping, it appears a NULL pointer dereference will occur:

drivers/of/base.c:of_parse_phandle_with_args_map() {
    ...
    /* Get the #<list>-cells property */
    cur = out_args->np;
    ...
}

Can this cause a panic if out_args is NULL?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-schneider-v7-2-rc1-eip201-upstream-v2-0-b4680787377e@bootlin.com?part=6