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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The driver fails to link when compiled as a module (`CONFIG_COMMON_CLK_SCMI=m`) because `clk-scmi.c` and `clk-scmi-oem.c` are built as separate modules without exporting their shared symbols.
- [High] Incorrect unit conversion for spread spectrum percentage causes valid spread values to be truncated to 0.

Pre-existing issues:
- [High] Missing bounds check on SCMI firmware-provided parent clock index leads to heap out-of-bounds read and wild pointer dereference.
--

commit 02412e4d55042034939e6eece844d1bd4d62d5e9
Author: Peng Fan <peng.fan@nxp.com>

clk: scmi: Add i.MX95 OEM extension support for SCMI clock driver

Introduces clk-scmi-oem.c to support vendor-specific OEM extensions for the
SCMI clock driver. This enables querying OEM-specific features and setting
spread spectrum configurations.

> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index 6a726331b6c9e..c2ae700ec0f2a 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -98,7 +98,7 @@ obj-$(CONFIG_COMMON_CLK_RP1)            += clk-rp1.o
>  obj-$(CONFIG_COMMON_CLK_RPMI)		+= clk-rpmi.o
>  obj-$(CONFIG_COMMON_CLK_HI655X)		+= clk-hi655x.o
>  obj-$(CONFIG_COMMON_CLK_S2MPS11)	+= clk-s2mps11.o
> -obj-$(CONFIG_COMMON_CLK_SCMI)           += clk-scmi.o
> +obj-$(CONFIG_COMMON_CLK_SCMI)           += clk-scmi.o clk-scmi-oem.o

[Severity: High]
Will this fail to link when compiled as a module (CONFIG_COMMON_CLK_SCMI=m)?

It looks like Kbuild will treat clk-scmi.o and clk-scmi-oem.o as separate
kernel modules rather than linking them into a single composite module.
Since neither file exports their shared symbols (like scmi_proto_clk_ops
or scmi_clk_oem_init), won't this result in undefined reference errors?

Should this use a composite module definition instead?

scmi-clk-y := clk-scmi.o clk-scmi-oem.o
obj-$(CONFIG_COMMON_CLK_SCMI) += scmi-clk.o

[ ... ]

> diff --git a/drivers/clk/clk-scmi-oem.c b/drivers/clk/clk-scmi-oem.c
> new file mode 100644
> index 0000000000000..be11d359b4ec3
> --- /dev/null
> +++ b/drivers/clk/clk-scmi-oem.c

[ ... ]

> +static int
> +scmi_clk_imx_set_spread_spectrum(struct clk_hw *hw,
> +				 const struct clk_spread_spectrum *ss_conf)
> +{
> +	struct scmi_clk *clk = to_scmi_clk(hw);
> +	int ret;
> +	u32 val;
> +
> +	/*
> +	 * extConfigValue[7:0]   - spread percentage (%)
> +	 * extConfigValue[23:8]  - Modulation Frequency
> +	 * extConfigValue[24]    - Enable/Disable
> +	 * extConfigValue[31:25] - Reserved
> +	 */
> +	val = FIELD_PREP(SCMI_CLOCK_IMX_SS_PERCENTAGE_MASK, ss_conf->spread_bp / 10000);

[Severity: High]
Does this integer division by 10000 inadvertently truncate valid spread
spectrum values to 0?

Since ss_conf->spread_bp is in basis points (where 100 basis points equals
1%), and standard configurations usually range from 50 to 500 basis points
(0.5% to 5%), dividing by 10000 will always result in zero. Could this
divide by 100 instead to correctly convert basis points to a percentage?

[ ... ]

> 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 is it possible for the SCMI firmware to
trigger an out-of-bounds read right after this ops setup?

Looking at scmi_clocks_probe(), it uses the parent index provided by the
SCMI firmware to directly access the dynamically allocated hws array:

drivers/clk/clk-scmi.c:scmi_clocks_probe() {
    ...
	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 provides an index greater than or equal to the total clock
count, won't this result in an out-of-bounds read on the heap, potentially
dereferencing a wild pointer? Should this index be verified against count
first?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612-clk-v10-v10-0-eb92484eda38@nxp.com?part=6