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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver dynamically allocates the clock array with insufficient memory due to a precedence error, and then writes to it using device tree values without bounds checking, causing an out-of-bounds write.
--

--- Patch [1]: [PATCH v2] dt-bindings: clock: ti,dm816-fapll-clock: Convert to DT schema ---
commit 4a9a2c7ea2387cee445b6eafff965419433f7d8c
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 the clock cells size,
and adds indices and output names as required properties.

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 @@
[ ... ]
> +  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 is a pre-existing issue, but since we're formalizing the clock-indices
constraints here, does the driver safely handle these index values?

Looking at drivers/clk/ti/fapll.c:ti_fapll_setup(), it allocates the clock
array without parentheses around the addition, resulting in an undersized
buffer:

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

Then it reads the clock-indices property from the device tree and uses it
as an array index without bounds checking:

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

Since MAX_FAPLL_OUTPUTS is 7, the allocation on a 32-bit architecture
requests 29 bytes (4 * 7 + 1) instead of the intended 32 bytes (4 * (7 + 1)).

When the valid index 7 is used from the device tree, does this write past
the end of the requested allocation? Furthermore, could a malformed device
tree providing an index greater than 7 cause memory corruption beyond the
slab boundary?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722-ti-fapll-clock-v2-1-085054acf2dc@gmail.com?part=1