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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing parentheses around addition causes an under-allocation of the `clks` array, leading to a heap buffer overflow.
- [High] Missing bounds check on `output_instance` read from the device tree before using it as an array index.
- [High] Incorrect `clk_num` tracking breaks clock retrieval for sparse or failed outputs.
--

commit df6f3e72211e7dd6f2410f0f3ef1e36b9dec0028
Author: Bhargav Joshi <j.bhargav.u@gmail.com>
Subject: dt-bindings: clock: ti,dm816-fapll-clock: Convert to DT schema

This commit converts the Texas Instruments DM816 FAPLL clock binding from
plain text to YAML DT schema. It strictly enforces #clock-cells = <1> and
adds clock-indices and clock-output-names as required properties based on
driver requirements.

Link: https://lore.kernel.org/r/20260717-ti-fapll-clock-v1-1-1b684cb2543b@gmail.com

> 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..423b7405e398e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ti/ti,dm816-fapll-clock.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/ti/ti,dm816-fapll-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#

[Severity: High]
This is a pre-existing issue, but I noticed a potential memory allocation
problem in the corresponding driver code when looking at the DT conversion.

In drivers/clk/ti/fapll.c:ti_fapll_setup(), there is an under-allocation
due to missing parentheses around the addition:

	fd->outputs.clks = kzalloc(sizeof(struct clk *) *
				   MAX_FAPLL_OUTPUTS + 1,
				   GFP_KERNEL);

The expression sizeof(struct clk *) * MAX_FAPLL_OUTPUTS + 1 evaluates to
57 bytes on a 64-bit architecture, but the code populates the array up to
index 7, which requires 64 bytes. Does this cause an out-of-bounds write
that corrupts adjacent heap memory when the driver probes?

[ ... ]

> +  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: High]
This isn't a bug introduced by this patch, but there is a lack of bounds
checking on the device tree values read for this property.

In drivers/clk/ti/fapll.c:ti_fapll_setup(), output_instance is read from the
clock-indices property and used immediately for memory offsets and array
indexing:

		if (of_property_read_u32_index(node, "clock-indices", i,
					       &output_instance))
			output_instance = i;

		freq = fd->base + (output_instance * 8);

and later:
		fd->outputs.clks[output_instance] = synth_clk;

If a malformed device tree provides an index larger than MAX_FAPLL_OUTPUTS,
could this cause an out-of-bounds heap read and write?

[Severity: High]
This is a pre-existing issue, but it could interact poorly with sparse
clock-indices from the device tree.

In drivers/clk/ti/fapll.c:ti_fapll_setup(), clk_num is incremented
sequentially for each successfully registered clock:

		fd->outputs.clks[output_instance] = synth_clk;
		fd->outputs.clk_num++;

Because of_clk_src_onecell_get() uses clk_num as the array size bound, if
the device tree defines sparse clock-indices or if a clock fails to register,
clk_num will be smaller than the highest registered output_instance index.
Will this cause valid clocks at higher indices to be rejected and become
inaccessible to consumers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906-ti-fapll-clock-v3-1-b0ded340c62b@gmail.com?part=1