[PATCH net-next 0/2] net: pcs: rzn1-miic: Validate dtb configuration values

[PATCH net-next 0/2] net: pcs: rzn1-miic: Validate dtb configuration values

[Kyle Hendry via B4 Relay]

This series addresses issues found when reviewing another fix:
https://lore.kernel.org/netdev/20260915-rzn1-miic-fix-array-v5-1-b7173fd5b97d@reliablecontrols.com/

Invalid values from the dtb could cause out of bounds array access. Checking
the values as they're parsed should prevent this.

Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>
---
Kyle Hendry (2):
      net: pcs: rzn1-miic: Make usage of miic_port_max consistent
      net: pcs: rzn1-miic: Validate dtb configuration values

 drivers/net/pcs/pcs-rzn1-miic.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)
---
base-commit: 42a9fb3382fc2573e92f41d203b095d9a372cfc9
change-id: 20260924-miic-validate-dtb-bfc4b380b782

Best regards,
-- 
Kyle Hendry <khendry@reliablecontrols.com>

[PATCH net-next 1/2] net: pcs: rzn1-miic: Make usage of miic_port_max consistent

[Kyle Hendry via B4 Relay]

From: Kyle Hendry <khendry@reliablecontrols.com>

miic_port_max is used both as the last port number and the port count
which can be different depending on SoC numbering. Use compile time
information to always set this as count and fix logic that was expecting
the last port number.

Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>
---
 drivers/net/pcs/pcs-rzn1-miic.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
index cb74861e823c..62daaf76e052 100644
--- a/drivers/net/pcs/pcs-rzn1-miic.c
+++ b/drivers/net/pcs/pcs-rzn1-miic.c
@@ -59,6 +59,8 @@
 
 #define MIIC_MAX_NUM_RSTS		2
 
+#define MIIC_PORT_END(x) ((x)->miic_port_start + (x)->miic_port_max - 1)
+
 /**
  * struct modctrl_match - Matching table entry for  convctrl configuration
  *			  See section 8.2.1 of manual.
@@ -222,7 +224,7 @@ enum miic_type {
  * @index_to_string: String representations of the index values
  * @index_to_string_count: Number of entries in the index_to_string array
  * @miic_port_start: MIIC port start number
- * @miic_port_max: Maximum MIIC supported
+ * @miic_port_max: Count of total MIIC ports supported
  * @sw_mode_mask: Switch mode mask
  * @reset_ids: Reset names array
  * @reset_count: Number of entries in the reset_ids array
@@ -482,7 +484,7 @@ struct phylink_pcs *miic_create(struct device *dev, struct device_node *np)
 
 	miic = platform_get_drvdata(pdev);
 	of_data = miic->of_data;
-	if (port > of_data->miic_port_max || port < of_data->miic_port_start) {
+	if (port > MIIC_PORT_END(of_data) || port < of_data->miic_port_start) {
 		put_device(&pdev->dev);
 		return ERR_PTR(-EINVAL);
 	}
@@ -822,7 +824,7 @@ static struct miic_of_data rzn1_miic_of_data = {
 	.index_to_string = index_to_string,
 	.index_to_string_count = ARRAY_SIZE(index_to_string),
 	.miic_port_start = 1,
-	.miic_port_max = 5,
+	.miic_port_max = ARRAY_SIZE(index_to_string) - 1,
 	.sw_mode_mask = GENMASK(4, 0),
 	.init_unlock_lock_regs = true,
 	.miic_write = miic_reg_writel_unlocked,
@@ -838,7 +840,7 @@ static struct miic_of_data rzt2h_miic_of_data = {
 	.index_to_string = rzt2h_index_to_string,
 	.index_to_string_count = ARRAY_SIZE(rzt2h_index_to_string),
 	.miic_port_start = 0,
-	.miic_port_max = 4,
+	.miic_port_max = ARRAY_SIZE(rzt2h_index_to_string) - 1,
 	.sw_mode_mask = GENMASK(2, 0),
 	.reset_ids = rzt2h_reset_ids,
 	.reset_count = ARRAY_SIZE(rzt2h_reset_ids),

-- 
2.43.0

Re: [PATCH net-next 1/2] net: pcs: rzn1-miic: Make usage of miic_port_max consistent

[Geert Uytterhoeven]

Hi Kyle,

On Fri, 25 Sept 2026 at 17:20, Kyle Hendry via B4 Relay
<devnull+khendry.reliablecontrols.com@kernel.org> wrote:
> From: Kyle Hendry <khendry@reliablecontrols.com>
>
> miic_port_max is used both as the last port number and the port count
> which can be different depending on SoC numbering. Use compile time
> information to always set this as count and fix logic that was expecting
> the last port number.
>
> Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>

Thanks for your patch!

> --- a/drivers/net/pcs/pcs-rzn1-miic.c
> +++ b/drivers/net/pcs/pcs-rzn1-miic.c
> @@ -59,6 +59,8 @@
>
>  #define MIIC_MAX_NUM_RSTS              2
>
> +#define MIIC_PORT_END(x) ((x)->miic_port_start + (x)->miic_port_max - 1)
> +
>  /**
>   * struct modctrl_match - Matching table entry for  convctrl configuration
>   *                       See section 8.2.1 of manual.
> @@ -222,7 +224,7 @@ enum miic_type {
>   * @index_to_string: String representations of the index values
>   * @index_to_string_count: Number of entries in the index_to_string array
>   * @miic_port_start: MIIC port start number
> - * @miic_port_max: Maximum MIIC supported
> + * @miic_port_max: Count of total MIIC ports supported

miic_port_num_total?
"max" has a different meaning.

>   * @sw_mode_mask: Switch mode mask
>   * @reset_ids: Reset names array
>   * @reset_count: Number of entries in the reset_ids array
> @@ -482,7 +484,7 @@ struct phylink_pcs *miic_create(struct device *dev, struct device_node *np)
>
>         miic = platform_get_drvdata(pdev);
>         of_data = miic->of_data;
> -       if (port > of_data->miic_port_max || port < of_data->miic_port_start) {
> +       if (port > MIIC_PORT_END(of_data) || port < of_data->miic_port_start) {

IMHO the asymmetry makes the code harder to read.

As this changes the logic, I assume this is a fix?

>                 put_device(&pdev->dev);
>                 return ERR_PTR(-EINVAL);
>         }
> @@ -822,7 +824,7 @@ static struct miic_of_data rzn1_miic_of_data = {
>         .index_to_string = index_to_string,
>         .index_to_string_count = ARRAY_SIZE(index_to_string),
>         .miic_port_start = 1,
> -       .miic_port_max = 5,
> +       .miic_port_max = ARRAY_SIZE(index_to_string) - 1,

Why the -1? Oh, because the first entry of the array is not included.

>         .sw_mode_mask = GENMASK(4, 0),
>         .init_unlock_lock_regs = true,
>         .miic_write = miic_reg_writel_unlocked,
> @@ -838,7 +840,7 @@ static struct miic_of_data rzt2h_miic_of_data = {
>         .index_to_string = rzt2h_index_to_string,
>         .index_to_string_count = ARRAY_SIZE(rzt2h_index_to_string),
>         .miic_port_start = 0,
> -       .miic_port_max = 4,
> +       .miic_port_max = ARRAY_SIZE(rzt2h_index_to_string) - 1,

Why the -1? Oh, because the first entry of the array is not included.
And it is not related to .miic_port_start, which is zero here?

>         .sw_mode_mask = GENMASK(2, 0),
>         .reset_ids = rzt2h_reset_ids,
>         .reset_count = ARRAY_SIZE(rzt2h_reset_ids),

I'm not sure this is an improvement at all...

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

Re: [PATCH net-next 1/2] net: pcs: rzn1-miic: Make usage of miic_port_max consistent

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message describes this patch as a consistency cleanup
  ("Make usage of miic_port_max consistent").

Pre-existing issues:
- [Medium] In the reviewed commit, miic_parse_dt() reads the per-
  converter DT "reg" into `int port` and never checks it against the…

--

net: pcs: rzn1-miic: Make usage of miic_port_max consistent

This patch makes miic_port_max always mean a port count rather than the
last port number. It adds a MIIC_PORT_END() helper for the last valid port
and uses it for the range check in miic_create(). The per-SoC values now
come from the index_to_string tables.

> diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
> index cb74861e823c8..62daaf76e0525 100644
> --- a/drivers/net/pcs/pcs-rzn1-miic.c
> +++ b/drivers/net/pcs/pcs-rzn1-miic.c
> @@ -59,6 +59,8 @@
>  
>  #define MIIC_MAX_NUM_RSTS		2
>  
> +#define MIIC_PORT_END(x) ((x)->miic_port_start + (x)->miic_port_max - 1)
> +
>  /**
>   * struct modctrl_match - Matching table entry for  convctrl configuration
>   *			  See section 8.2.1 of manual.

[ ... ]

> @@ -482,7 +484,7 @@ struct phylink_pcs *miic_create(struct device *dev, struct device_node *np)
>  
>  	miic = platform_get_drvdata(pdev);
>  	of_data = miic->of_data;
> -	if (port > of_data->miic_port_max || port < of_data->miic_port_start) {
> +	if (port > MIIC_PORT_END(of_data) || port < of_data->miic_port_start) {
>  		put_device(&pdev->dev);
>  		return ERR_PTR(-EINVAL);
>  	}

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Now that
MIIC_PORT_END() exists, should miic_parse_dt() use it as well?

miic_parse_dt() reads each converter's reg into int port and uses it with
no range check:

drivers/net/pcs/pcs-rzn1-miic.c:miic_parse_dt() {
    ...
	for_each_available_child_of_node(np, conv) {
		if (of_property_read_u32(conv, "reg", &port))
			continue;
		...
		/* Adjust for 0 based index */
		dt_val[port + !miic->of_data->miic_port_start] = conf;
		...
		miic_configure_phy_link(miic, conf, port, active_low);
	}
    ...
}

dt_val is allocated with conf_conv_count entries (6 on RZ/N1, 5 on
RZ/T2H). On RZ/T2H, reg = 4 writes dt_val[5], one byte past the end of the
allocation. Larger or negative reg values write further out of bounds.

miic_configure_phy_link() also builds BIT(port + shift) from the same
unchecked value, and the result is written to MIIC_PHY_LINK.

The miic_probe()->miic_parse_dt() path runs before miic_create() sees the
port, so the new check here does not cover it. Only a malformed DT can
reach this, since the binding restricts reg per SoC.

A later patch in this series, "net: pcs: rzn1-miic: Validate dtb
configuration values", appears to add a port < miic_port_start ||
port > MIIC_PORT_END() check to miic_parse_dt() and fail probe with
-EINVAL. That would resolve this.

[ ... ]

> @@ -838,7 +840,7 @@ static struct miic_of_data rzt2h_miic_of_data = {
>  	.index_to_string = rzt2h_index_to_string,
>  	.index_to_string_count = ARRAY_SIZE(rzt2h_index_to_string),
>  	.miic_port_start = 0,
> -	.miic_port_max = 4,
> +	.miic_port_max = ARRAY_SIZE(rzt2h_index_to_string) - 1,
>  	.sw_mode_mask = GENMASK(2, 0),
>  	.reset_ids = rzt2h_reset_ids,
>  	.reset_count = ARRAY_SIZE(rzt2h_reset_ids),

[Severity: Low]
The subject describes this as a consistency cleanup:

  net: pcs: rzn1-miic: Make usage of miic_port_max consistent

Isn't this really a functional fix that only affects RZ/T2H and RZ/N2H?

The value here is still 4, but its meaning has changed. Before this patch,
miic_init_hw() already used it as a count:

	for (port = 0; port < miic->of_data->miic_port_max; port++) {

miic_create(), on the other hand, used it as the last valid port:

	if (port > of_data->miic_port_max || port < of_data->miic_port_start) {

So on RZ/T2H, a PCS node with reg = 4 was accepted, even though
rzt2h_index_to_string only lists CONV0..CONV3. That produced
miic_port->port = 4. The phylink ops then wrote MIIC_CONVCTRL(4) and
toggled bit 4 of CONVRST for a converter that does not exist.

After this patch, MIIC_PORT_END() is 0 + 4 - 1 = 3 on RZ/T2H, so
miic_create() now rejects reg = 4 with -EINVAL.

Nothing changes on RZ/N1. ARRAY_SIZE(index_to_string) - 1 is still 5, and
MIIC_PORT_END() is 1 + 5 - 1 = 5.

Could the commit message name the affected SoC, describe this change in
behaviour, and note that RZ/N1 is unaffected?

Should it also carry a Fixes: tag? The mismatch appears to come from
08f89e42121d ("net: pcs: rzn1-miic: Add RZ/T2H MIIC support"). That commit
added .miic_port_max = 4, while the check from c112520de041 ("net: pcs:
rzn1-miic: move port range handling into SoC data") treated the value as
the last port.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-miic-validate-dtb-v1-0-3a6db9bb75ec%40reliablecontrols.com

[PATCH net-next 2/2] net: pcs: rzn1-miic: Validate dtb configuration values

[Kyle Hendry via B4 Relay]

From: Kyle Hendry <khendry@reliablecontrols.com>

Bad configuration values from the dtb could result in out of bounds array
access. Verify that parsed values are within range for the SoC and fail the
probe if they are invalid.

Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>
---
 drivers/net/pcs/pcs-rzn1-miic.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
index 62daaf76e052..30ac2d407005 100644
--- a/drivers/net/pcs/pcs-rzn1-miic.c
+++ b/drivers/net/pcs/pcs-rzn1-miic.c
@@ -695,9 +695,24 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
 		if (of_property_read_u32(conv, "reg", &port))
 			continue;
 
+		if (port < miic->of_data->miic_port_start ||
+		    port > MIIC_PORT_END(miic->of_data)) {
+			dev_err(miic->dev, "Port number out of range: %d\n", port);
+			of_node_put(conv);
+			ret = -EINVAL;
+			goto err;
+		}
+
 		if (of_property_read_u32(conv, "renesas,miic-input", &conf))
 			continue;
 
+		if (conf >= miic->of_data->conf_to_string_count) {
+			dev_err(miic->dev, "Port configuration out of range: %d\n", conf);
+			of_node_put(conv);
+			ret = -EINVAL;
+			goto err;
+			}
+
 		/* Adjust for 0 based index */
 		dt_val[port + !miic->of_data->miic_port_start] = conf;
 
@@ -707,6 +722,7 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
 	}
 
 	ret = miic_match_dt_conf(miic, dt_val, mode_cfg);
+err:
 	kfree(dt_val);
 
 	return ret;

-- 
2.43.0

Re: [PATCH net-next 2/2] net: pcs: rzn1-miic: Validate dtb configuration values

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 2 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete.
- [Low] The patch fixes a real memory-safety bug but has no Fixes: tag.
- [Low] Coding style in the new code in miic_parse_dt().

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but the kernel-doc
  for struct modctrl_match.conv says that for RZ/T2H/RZ/N2H 'the first…

--

net: pcs: rzn1-miic: Validate dtb configuration values

This patch adds range checks in miic_parse_dt() for the "reg" and
"renesas,miic-input" values read from child nodes. If either value is
out of range for the SoC, the probe fails with -EINVAL.

> Bad configuration values from the dtb could result in out of bounds array
> access. Verify that parsed values are within range for the SoC and fail the
> probe if they are invalid.
>
> Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>

[Severity: Low]
Should this have a Fixes: tag?

Before this patch, miic_parse_dt() used an unchecked DT "reg" value here:

	dt_val[port + !miic->of_data->miic_port_start] = conf;

That could write outside the conf_conv_count byte buffer. The buffer was
originally a fixed on-stack array. This unchecked write and the
conf_to_string[conf[i]] lookup both appear to date back to commit
7dc54d3b8d91 ("net: pcs: add Renesas MII converter driver").

The earlier commit daf677c2c644 ("net: pcs: rzn1-miic: Fix config array
initialization") has a Fixes: tag. Would something like this work here?

Fixes: 7dc54d3b8d91 ("net: pcs: add Renesas MII converter driver")

> diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
> index 62daaf76e0525..30ac2d4070052 100644
> --- a/drivers/net/pcs/pcs-rzn1-miic.c
> +++ b/drivers/net/pcs/pcs-rzn1-miic.c
> @@ -695,9 +695,24 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)

[ ... ]

>  		if (of_property_read_u32(conv, "renesas,miic-input", &conf))
>  			continue;
>  
> +		if (conf >= miic->of_data->conf_to_string_count) {
> +			dev_err(miic->dev, "Port configuration out of range: %d\n", conf);
> +			of_node_put(conv);
> +			ret = -EINVAL;
> +			goto err;
> +			}

[Severity: High]
The commit message says the patch will "Verify that parsed values are
within range for the SoC". Is the renesas,miic-switch-portin value still
unchecked? Earlier in miic_parse_dt() it is stored directly:

	if (of_property_read_u32(np, "renesas,miic-switch-portin", &conf) == 0)
		dt_val[0] = conf;

Take an out-of-range value, such as 20 on RZ/N1 (where conf_to_string[]
has 14 entries) or 9 or more on RZ/T2H. No modctrl_match entry will
match it, so the error path uses it as an index:

miic_parse_dt()
  miic_match_dt_conf()
    miic_dump_conf()
      conf_name = of_data->conf_to_string[conf[i]];

Can this read past the end of conf_to_string[] and then pass the pointer
it finds to dev_err() as a %s string?

dt_val is also s8, so the u32 is truncated with no check:

- 128 to 254 become negative indexes.
- 255 becomes MIIC_MODCTRL_CONF_NONE, so the property is silently
  ignored.
- 258 wraps to 2 and is accepted as a valid value.

The binding limits this property to enum [0, 1, 2], but that is only
checked by dt_binding_check, not at runtime.

Could the same range check be applied to the u32 value before it is
stored in dt_val[0]? Checking it against the PORTIN values the SoC
allows would be stricter still.

[Severity: Low]
This isn't a bug, but the closing brace of this new block is indented one
tab too far.

Also, conf is a u32. Should the dev_err() use %u instead of %d?

> +
>  		/* Adjust for 0 based index */
>  		dt_val[port + !miic->of_data->miic_port_start] = conf;

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The
kernel-doc for struct modctrl_match describes RZ/T2H and RZ/N2H like
this:

 * @conv: Configuration of ethernet port muxes. First index is SWITCH_PORTIN,
 *	  then index 1 - 5 are CONV1 - CONV5 for RZ/N1 SoCs. In case
 *	  of RZ/T2H and RZ/N2H SoCs, the first index is SWITCH_PORTIN then
 *	  index 0 - 3 are CONV0 - CONV3.

Index 0 is already SWITCH_PORTIN. This line stores RZ/T2H converter p at
dt_val[p + 1], which matches rzt2h_index_to_string[] being
{"SWITCH_PORTIN", "CONV0", ..., "CONV3"}.

Should the comment say that index 1 - 4 are CONV0 - CONV3?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-miic-validate-dtb-v1-0-3a6db9bb75ec%40reliablecontrols.com