Re: [PATCH] dt-bindings: clock: ti,dm816-fapll-clock: Convert to DT schema

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Operator precedence bug causes an undersized array allocation leading to an out-of-bounds write
- [High] Incorrect array bounds tracking breaks clock lookup for sparse clock configurations
--

--- Patch [1]: [PATCH] dt-bindings: clock: ti,dm816-fapll-clock: Convert to DT schema ---
commit 87daba9f6927df76d909d0fe50168fefb82110f5
Author: Bhargav Joshi <j.bhargav.u@gmail.com>

dt-bindings: clock: ti,dm816-fapll-clock: Convert to DT schema

Convert the Texas Instruments DM816 FAPLL clock binding from plain text
to YAML DT schema. The schema now strictly enforces #clock-cells = <1>,
and adds clock-indices and clock-output-names as required properties.

> diff --git a/Documentation/devicetree/bindings/clock/ti/ti,dm816-fapll-clock.yaml b/Documentation/devicetree/bindings/clock/ti/ti,dm816-fapll-clock.yaml
> new file mode 100644
> index 0000000000000..9b6b824faa305
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ti/ti,dm816-fapll-clock.yaml
[ ... ]
> +  clock-indices:
> +    description:
> +      Maps child synthesizers to physical indices 1-7. Index 0 is physically
> +      reserved for the Master PLL.
> +    minItems: 1
> +    maxItems: 7
> +    items:
> +      minimum: 1
> +      maximum: 7

[Severity: Critical]
This is a pre-existing issue, but by officially documenting 7 as the maximum
index, this binding exposes an out-of-bounds write in the driver.

In the driver code:

drivers/clk/ti/fapll.c:ti_fapll_setup() {
    ...
        fd->outputs.clks = kzalloc(sizeof(struct clk *) *
                                   MAX_FAPLL_OUTPUTS + 1,
                                   GFP_KERNEL);
    ...
}

Due to C operator precedence, this evaluates to (8 * 7) + 1 = 57 bytes on
64-bit architectures, rather than the intended 8 * (7 + 1) = 64 bytes.

If a device tree uses a clock index of 7 (the maximum allowed, as demonstrated
in the DT bindings example), the driver writes to fd->outputs.clks[7]. This
pointer write covers bytes 56 through 63, overflowing the 57-byte allocation by
7 bytes, which can cause memory corruption and a KASAN splat.

Could we fix the operator precedence in the driver by adding parentheses around
MAX_FAPLL_OUTPUTS + 1?

[Severity: High]
This isn't a bug introduced by this patch, but there's a pre-existing issue in
how the driver handles the sparse indices allowed by this binding.

drivers/clk/ti/fapll.c:ti_fapll_setup() {
    ...
        fd->outputs.clks[output_instance] = synth_clk;
        fd->outputs.clk_num++;
    ...
}

If the device tree specifies sparse indices (e.g., <1> and <5>), clk_num is
incremented to 3 instead of tracking the maximum index + 1.

When of_clk_src_onecell_get() performs a lookup for index 5, it checks
if (idx >= clk_data->clk_num) (i.e., 5 >= 3), which incorrectly rejects the
valid lookup and returns -EINVAL.

Can the driver be updated to set clk_num to the maximum index + 1 instead of
just counting the number of registered clocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-ti-fapll-clock-v1-1-1b684cb2543b@gmail.com?part=1