[PATCH v5 0/2] clk: rs9: Fix and of_clk_hw_onecell_get() conversion

[PATCH v5 0/2] clk: rs9: Fix and of_clk_hw_onecell_get() conversion

[Geert Uytterhoeven]

	Hi all,

This patch series contains a fix and further development for the Renesas
9-series PCIe clock generator driver, which were submitted before as
separate patches  As the fix hasn't been picked up yet, and the further
development depends on the fix, I am resending them as a series.

Changes compared to v4[1]:
  - Add Reviewed-by.

Thanks for your comments!

[1] "[PATCH v4 0/2] clk: rs9: Fix and of_clk_hw_onecell_get() conversion"
    https://lore.kernel.org/r/cover.1773241119.git.geert+renesas@glider.be

Geert Uytterhoeven (2):
  clk: rs9: Add clock index range check to rs9_of_clk_get()
  clk: rs9: Convert to clk_hw_onecell_data and of_clk_hw_onecell_get()

 drivers/clk/clk-renesas-pcie.c | 39 ++++++++++++++++------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

-- 
2.43.0

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

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

[Geert Uytterhoeven]

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().

Fixes: 892e0ddea1aa6f70 ("clk: rs9: Add Renesas 9-series PCIe clock generator driver")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
v5:
  - Add Reviewed-by,

v4:
  - Put in a series,

v3:
  - Add error message,

v2:
  - Just add the missing range check; the conversion to
    of_clk_hw_onecell_get() can be done later.

v1: "[PATCH] clk: rs9: Convert to clk_hw_onecell_data and
     of_clk_hw_onecell_get()"
    https://lore.kernel.org/a6dce17b15d29a257d09fe0edc199a14c297f1a8.1768836042.git.geert+renesas@glider.be
---
 drivers/clk/clk-renesas-pcie.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index 564550c803896756..f3376c29007aa92d 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];
 }
 
-- 
2.43.0

[PATCH v5 2/2] clk: rs9: Convert to clk_hw_onecell_data and of_clk_hw_onecell_get()

[Geert Uytterhoeven]

Convert the rs9 clock driver to use the of_clk_hw_onecell_get() helper,
which requires using the clk_hw_onecell_data structure.  Embedding that
structure in the rs9_driver_data structure has the benefit that the
clock array always has the correct size, and thus can no longer become
out of sync when adding support for new rs9 variants.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
v5:
  - No changes,

v4:
  - Add Reviewed-by,
  - Rebase on top of "clk: rs9: Add clock index range check to
    rs9_of_clk_get()", so this becomes a pure refactoring instead of a
    fix,
  - Put in a series,

(v3 and v2 do not exist)

v1:
  - https://lore.kernel.org/a6dce17b15d29a257d09fe0edc199a14c297f1a8.1768836042.git.geert+renesas@glider.be
---
 drivers/clk/clk-renesas-pcie.c | 44 ++++++++++++++--------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/drivers/clk/clk-renesas-pcie.c b/drivers/clk/clk-renesas-pcie.c
index f3376c29007aa92d..e2b441f99ccab4c2 100644
--- a/drivers/clk/clk-renesas-pcie.c
+++ b/drivers/clk/clk-renesas-pcie.c
@@ -63,10 +63,11 @@ struct rs9_driver_data {
 	struct i2c_client	*client;
 	struct regmap		*regmap;
 	const struct rs9_chip_info *chip_info;
-	struct clk_hw		*clk_dif[8];
 	u8			pll_amplitude;
 	u8			pll_ssc;
 	u8			clk_dif_sr;
+	/* must be last */
+	struct clk_hw_onecell_data onecell;
 };
 
 /*
@@ -270,37 +271,28 @@ static void rs9_update_config(struct rs9_driver_data *rs9)
 	}
 }
 
-static struct clk_hw *
-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];
-}
-
 static int rs9_probe(struct i2c_client *client)
 {
+	const struct rs9_chip_info *chip_info;
 	unsigned char name[5] = "DIF0";
 	struct rs9_driver_data *rs9;
 	unsigned int vid, did;
 	struct clk_hw *hw;
 	int i, ret;
 
-	rs9 = devm_kzalloc(&client->dev, sizeof(*rs9), GFP_KERNEL);
+	chip_info = i2c_get_match_data(client);
+	if (!chip_info)
+		return -EINVAL;
+
+	rs9 = devm_kzalloc(&client->dev, struct_size(rs9, onecell.hws,
+			   chip_info->num_clks), GFP_KERNEL);
 	if (!rs9)
 		return -ENOMEM;
 
 	i2c_set_clientdata(client, rs9);
 	rs9->client = client;
-	rs9->chip_info = i2c_get_match_data(client);
-	if (!rs9->chip_info)
-		return -EINVAL;
+	rs9->chip_info = chip_info;
+	rs9->onecell.num = chip_info->num_clks;
 
 	/* Fetch common configuration from DT (if specified) */
 	ret = rs9_get_common_config(rs9);
@@ -308,7 +300,7 @@ static int rs9_probe(struct i2c_client *client)
 		return ret;
 
 	/* Fetch DIFx output configuration from DT (if specified) */
-	for (i = 0; i < rs9->chip_info->num_clks; i++) {
+	for (i = 0; i < rs9->onecell.num; i++) {
 		ret = rs9_get_output_config(rs9, i);
 		if (ret)
 			return ret;
@@ -334,24 +326,24 @@ static int rs9_probe(struct i2c_client *client)
 		return ret;
 
 	vid &= RS9_REG_VID_MASK;
-	if (vid != RS9_REG_VID_IDT || did != rs9->chip_info->did)
+	if (vid != RS9_REG_VID_IDT || did != chip_info->did)
 		return dev_err_probe(&client->dev, -ENODEV,
 				     "Incorrect VID/DID: %#02x, %#02x. Expected %#02x, %#02x\n",
-				     vid, did, RS9_REG_VID_IDT,
-				     rs9->chip_info->did);
+				     vid, did, RS9_REG_VID_IDT, chip_info->did);
 
 	/* Register clock */
-	for (i = 0; i < rs9->chip_info->num_clks; i++) {
+	for (i = 0; i < rs9->onecell.num; i++) {
 		snprintf(name, 5, "DIF%d", i);
 		hw = devm_clk_hw_register_fixed_factor_index(&client->dev, name,
 						    0, 0, 4, 1);
 		if (IS_ERR(hw))
 			return PTR_ERR(hw);
 
-		rs9->clk_dif[i] = hw;
+		rs9->onecell.hws[i] = hw;
 	}
 
-	ret = devm_of_clk_add_hw_provider(&client->dev, rs9_of_clk_get, rs9);
+	ret = devm_of_clk_add_hw_provider(&client->dev, of_clk_hw_onecell_get,
+					  &rs9->onecell);
 	if (!ret)
 		rs9_update_config(rs9);
 
-- 
2.43.0

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

[sashiko-bot]

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