Re: [PATCH v5 1/2] clk: rs9: Add clock index range check to rs9_of_clk_get()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Hardware configuration is applied after exposing the clock provider to consumers.
- [High] Stack-allocated buffers are used for I2C transfers, violating DMA safety requirements.
- [High] Probe fails entirely if an optional Device Tree property is missing.
--

commit 1a5bae9844abc7b4414cda261d8a9d3cca8fb310
Author: Geert Uytterhoeven <geert+renesas@glider.be>

clk: rs9: Add clock index range check to rs9_of_clk_get()

rs9_of_clk_get() does not validate the clock index in the passed
DT clock specifier.  If DT specifies an incorrect and out-of-range
index, this will access memory beyond the end of the clk_dif[] array.

Fix by this adding a range check to rs9_of_clk_get().

> diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
> index 564550c803896..f3376c29007aa 100644
> --- a/drivers/clk/clk-renesas-pcie.c
> +++ b/drivers/clk/clk-renesas-pcie.c

[ ... ]

> @@ -276,6 +276,11 @@ rs9_of_clk_get(struct of_phandle_args *clkspec, void *data)
>  	struct rs9_driver_data *rs9 = data;
>  	unsigned int idx = clkspec->args[0];
>  
> +	if (idx >= rs9->chip_info->num_clks) {
> +		pr_err("%s: Invalid clock index %u\n", __func__, idx);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
>  	return rs9->clk_dif[idx];
>  }

While reviewing this patch, I noticed a few pre-existing issues in the same
file.

drivers/clk/clk-renesas-pcie.c:rs9_probe() {
    ...
	ret = devm_of_clk_add_hw_provider(&client->dev, of_clk_hw_onecell_get,
					  &rs9->onecell);
	if (!ret)
		rs9_update_config(rs9);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the hardware
configuration happen too late during probe?

Since rs9_probe() registers the clock provider via
devm_of_clk_add_hw_provider(), exposing it to the system, could a
concurrently probing consumer acquire and use the unconfigured clock before
rs9_update_config() applies the hardware parameters?


drivers/clk/clk-renesas-pcie.c:rs9_regmap_i2c_write() {
    ...
	const u8 data[3] = { reg, 1, val };
	const int count = ARRAY_SIZE(data);
	int ret;

	ret = i2c_master_send(i2c, data, count);
    ...
}

And similarly in rs9_regmap_i2c_read():

drivers/clk/clk-renesas-pcie.c:rs9_regmap_i2c_read() {
    ...
	u8 txdata = reg;
	u8 rxdata[2];

	...
	xfer[0].buf = (void *)&txdata;

	...
	xfer[1].buf = (void *)rxdata;

	ret = i2c_transfer(i2c->adapter, xfer, 2);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but do the custom I2C read and
write functions pass stack-allocated buffers to the I2C core?

If the underlying I2C adapter relies on DMA mapping, could passing these
stack-allocated buffers (data, txdata, rxdata) to i2c_master_send() and
i2c_transfer() trigger a panic or silent memory corruption on architectures
with VMAP_STACK enabled?


drivers/clk/clk-renesas-pcie.c:rs9_get_output_config() {
    ...
	ret = of_property_read_u32(np, "renesas,slew-rate", &sr);
	of_node_put(np);
	if (!ret) {
		...
	}

	return ret;
}

[Severity: High]
This isn't a bug introduced by this patch, but can missing an optional
Device Tree property cause the probe to fail entirely?

If the optional "renesas,slew-rate" property is absent from a child node,
of_property_read_u32() returns -EINVAL. Could this unconditionally propagate
back through rs9_probe() and break the clock generator completely, instead
of safely falling back to default settings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788429994.git.geert+renesas@glider.be?part=1