Re: [PATCH v11 6/6] clk: scmi: Add i.MX95 OEM extension support for SCMI clock driver

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

Pre-existing issues:
- [High] Out-of-bounds read on `hws` array due to missing bounds check on firmware-supplied parent indices.
--

--- Patch [6]: [PATCH v11 6/6] clk: scmi: Add i.MX95 OEM extension support for SCMI clock driver ---
commit d5aa65b1d50040a18c4b3cc2d72d625cca785922
Author: Peng Fan <[email protected]>

clk: scmi: Add i.MX95 OEM extension support for SCMI clock driver
    
This commit introduces OEM extension support for the SCMI clock driver to
allow vendor-specific features like spread spectrum configuration. It adds
a new file clk-scmi-oem.c, initializes the extensions via scmi_clk_oem_init(),
and passes the scmi_device to scmi_clk_ops_select() for OEM data access.

> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index d88e78cc9a12e..2dd50c5b4ea8f 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c

[ ... ]

> @@ -406,7 +419,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
>  		 * to avoid sharing the devm_ allocated clk_ops between multiple
>  		 * SCMI clk driver instances.
>  		 */
> -		scmi_ops = scmi_clk_ops_select(sclk, transport_is_atomic,
> +		scmi_ops = scmi_clk_ops_select(sdev, sclk, transport_is_atomic,
>  					       atomic_threshold_us,
>  					       scmi_clk_ops_db,
>  					       ARRAY_SIZE(scmi_clk_ops_db));

[Severity: High]
This is a pre-existing issue, but does this function lack bounds checking on
the firmware-supplied parent indices?

In scmi_clocks_probe(), the total number of clocks is retrieved from the SCMI
firmware, and the hws array is allocated to that size. During clock
initialization, the firmware is queried for possible parent clocks:

	if (sclk->info->num_parents > 0) {
		sclk->parent_data = devm_kcalloc(dev, sclk->info->num_parents,
						 sizeof(*sclk->parent_data), GFP_KERNEL);
		if (!sclk->parent_data)
			return -ENOMEM;

		for (int i = 0; i < sclk->info->num_parents; i++) {
			sclk->parent_data[i].index = sclk->info->parents[i];
			sclk->parent_data[i].hw = hws[sclk->info->parents[i]];
		}
	}

If the firmware returns a parent clock identifier (sclk->info->parents[i])
that is greater than or equal to the clock count, wouldn't the driver use it
without validation as an array index into the hws array?

Could this lead to an out-of-bounds memory read and a kernel crash when the
clock framework later dereferences the invalid pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6