[PATCH v4 0/2] net: pcs: rzn1-miic: Fix config array initialization

[PATCH v4 0/2] net: pcs: rzn1-miic: Fix config array initialization

[Kyle Hendry via B4 Relay]

Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>
---
Fix issues when populating dt_val array from device tree values

Changes in v4:
 * Fix lines longer than 80 char
 * Add of_node_put() before returning

Changes in v3: https://lore.kernel.org/r/20260902-rzn1-miic-fix-array-v3-0-3f7dccffaf5a@reliablecontrols.com
 * Make invalid port number return error
 * Update commit tags

Changes in v2: https://lore.kernel.org/r/20260901-rzn1-miic-fix-array-v2-0-3e907049e770@reliablecontrols.com
 * Use a more correct array size in memset
 * Ensure that write to dt_val is in range

Link to v1: https://lore.kernel.org/r/20260813-rzn1-miic-fix-array-v1-1-b58cafcc917e@reliablecontrols.com

---
Kyle Hendry (2):
      net: pcs: rzn1-miic: Fix config array initialization
      net: pcs: rzn1-miic: Verify port number from dtb

 drivers/net/pcs/pcs-rzn1-miic.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
---
base-commit: e6b6078ea1731b05b3b552497b3bce4bf8b014ae
change-id: 20260813-rzn1-miic-fix-array-e6ae4452c017

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

[PATCH v4 1/2] net: pcs: rzn1-miic: Fix config array initialization

[Kyle Hendry via B4 Relay]

From: Kyle Hendry <khendry@reliablecontrols.com>

Fix memset parameters to initialize the entire DT value array

Fixes: f39e968dc168a7bd ("net: pcs: rzn1-miic: Move configuration data to SoC-specific struct")
Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>
---
 drivers/net/pcs/pcs-rzn1-miic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
index 2b72fa98ddf1..cb74861e823c 100644
--- a/drivers/net/pcs/pcs-rzn1-miic.c
+++ b/drivers/net/pcs/pcs-rzn1-miic.c
@@ -683,7 +683,8 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
 	if (!dt_val)
 		return -ENOMEM;
 
-	memset(dt_val, MIIC_MODCTRL_CONF_NONE, sizeof(*dt_val));
+	memset(dt_val, MIIC_MODCTRL_CONF_NONE,
+	       sizeof(*dt_val) * miic->of_data->conf_conv_count);
 
 	if (of_property_read_u32(np, "renesas,miic-switch-portin", &conf) == 0)
 		dt_val[0] = conf;

-- 
2.43.0

Re: [PATCH v4 1/2] net: pcs: rzn1-miic: Fix config array initialization

[Geert Uytterhoeven]

On Mon, 14 Sept 2026 at 18:41, Kyle Hendry via B4 Relay
<devnull+khendry.reliablecontrols.com@kernel.org> wrote:
> From: Kyle Hendry <khendry@reliablecontrols.com>
>
> Fix memset parameters to initialize the entire DT value array
>
> Fixes: f39e968dc168a7bd ("net: pcs: rzn1-miic: Move configuration data to SoC-specific struct")
> Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>

My
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
is still valid.

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 v4 1/2] net: pcs: rzn1-miic: Fix config array initialization

[Andrew Lunn]

On Mon, Sep 14, 2026 at 09:41:00AM -0700, Kyle Hendry via B4 Relay wrote:
> From: Kyle Hendry <khendry@reliablecontrols.com>
> 
> Fix memset parameters to initialize the entire DT value array
> 
> Fixes: f39e968dc168a7bd ("net: pcs: rzn1-miic: Move configuration data to SoC-specific struct")
> Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v4 1/2] net: pcs: rzn1-miic: Fix config array initialization

[Andrew Lunn]

On Mon, Sep 14, 2026 at 09:41:00AM -0700, Kyle Hendry via B4 Relay wrote:
> From: Kyle Hendry <khendry@reliablecontrols.com>
> 
> Fix memset parameters to initialize the entire DT value array
> 
> Fixes: f39e968dc168a7bd ("net: pcs: rzn1-miic: Move configuration data to SoC-specific struct")
> Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>

Actually...

Please split this patch out and submit it to net, not net-next. It is
a real fix.

  Andrew

Re: [PATCH v4 1/2] net: pcs: rzn1-miic: Fix config array initialization

[netdev-bot+sashiko]

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

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

- [High] One-byte heap out-of-bounds write in miic_parse_dt() on the
  RZ/T2H (renesas,r9a09g077-miic) variant, which the series' own port-…

Pre-existing issues:
- [High] Unbounded array index / wild pointer dereference in
  miic_dump_conf() (drivers/net/pcs/pcs-rzn1-miic.c:566-580).

--

net: pcs: rzn1-miic: Fix config array initialization

This corrects the memset() in miic_parse_dt() so that the whole
kmalloc_objs() allocated dt_val array is initialized to
MIIC_MODCTRL_CONF_NONE, instead of just the first byte. While looking at
the surrounding code a couple of questions came up about how dt_val is
indexed and later printed.

> diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
> index 2b72fa98ddf13..cb74861e823c8 100644
> --- a/drivers/net/pcs/pcs-rzn1-miic.c
> +++ b/drivers/net/pcs/pcs-rzn1-miic.c
> @@ -683,7 +683,8 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
>  	if (!dt_val)
>  		return -ENOMEM;
>  
> -	memset(dt_val, MIIC_MODCTRL_CONF_NONE, sizeof(*dt_val));
> +	memset(dt_val, MIIC_MODCTRL_CONF_NONE,
> +	       sizeof(*dt_val) * miic->of_data->conf_conv_count);
>  
>  	if (of_property_read_u32(np, "renesas,miic-switch-portin", &conf) == 0)
>  		dt_val[0] = conf;

[Severity: High]
Now that the size of this buffer is spelled out as conf_conv_count bytes,
can the index used a few lines further down in miic_parse_dt() run one
past the end on the RZ/T2H variant?

	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;

For rzt2h_miic_of_data the relevant fields are:

	.conf_conv_count = 5,
	.miic_port_start = 0,
	.miic_port_max = 4,

With miic_port_start == 0 the index is port + 1, so a child node with
reg = <4> and a renesas,miic-input property writes dt_val[5] in a 5 byte
allocation. The companion patch "net: pcs: rzn1-miic: Verify port number
from dtb" bounds port by miic_port_max, but since miic_port_max is 4 for
RZ/T2H the reg = <4> case still passes the check and still writes one byte
past the allocation.

Is miic_port_max = 4 intended here? The hardware description elsewhere in
the driver only covers four converters:

	static const char * const rzt2h_index_to_string[] = {
		"SWITCH_PORTIN",
		"CONV0",
		"CONV1",
		"CONV2",
		"CONV3",
	};

and the binding restricts reg to enum: [0, 1, 2, 3] for the r9a09g077
compatibles. Would bounding the computed index by conf_conv_count (or
setting miic_port_max = 3 for RZ/T2H) be more appropriate?

On RZ/N1 the same expression stays in range because miic_port_start == 1
makes the index equal to port, with a maximum of 5 in a 6 entry array.

[Severity: High]
This isn't a bug introduced by this patch, but since the same dt_val array
is involved: the values stored here are used directly as a string table
index in miic_dump_conf(), without any range check.

miic_parse_dt() stores the raw u32 DT properties into the s8 array:

	if (of_property_read_u32(np, "renesas,miic-switch-portin", &conf) == 0)
		dt_val[0] = conf;
	...
		dt_val[port + !miic->of_data->miic_port_start] = conf;

and when no MODCTRL entry matches, miic_match_dt_conf() unconditionally
dumps them:

	dev_err(miic->dev, "Failed to apply requested configuration\n");
	miic_dump_conf(miic, dt_val);

miic_dump_conf() then does:

	for (i = 0; i < of_data->conf_conv_count; i++) {
		if (conf[i] != MIIC_MODCTRL_CONF_NONE)
			conf_name = of_data->conf_to_string[conf[i]];
		...
		dev_err(miic->dev, "%s: %s\n",
			of_data->index_to_string[i], conf_name);

conf_to_string has 14 valid indices (0..MIIC_HSR_PORTB) and
rzt2h_conf_to_string has 9 (0..ETHSS_ETHSW_PORT2). A DT value outside
those ranges never matches a table row, so it always reaches this path,
and a value above 127 is truncated to a negative s8. Can this read a
pointer sized word from outside the string table and then dereference it
in the %s conversion?

struct miic_of_data already carries the bound, but it does not appear to be
read anywhere:

	u8 conf_to_string_count;
	...
	u8 index_to_string_count;

Would it make sense to validate conf[i] against conf_to_string_count
before the lookup, or to reject out of range renesas,miic-input and
renesas,miic-switch-portin values in miic_parse_dt()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914-rzn1-miic-fix-array-v4-0-069112feec86%40reliablecontrols.com

[PATCH v4 2/2] net: pcs: rzn1-miic: Verify port number from dtb

[Kyle Hendry via B4 Relay]

From: Kyle Hendry <khendry@reliablecontrols.com>

Add check to make sure port number is in range before writing
to dt_val array

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

diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
index cb74861e823c..1bd95ddf9400 100644
--- a/drivers/net/pcs/pcs-rzn1-miic.c
+++ b/drivers/net/pcs/pcs-rzn1-miic.c
@@ -693,6 +693,14 @@ 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->of_data->miic_port_max) {
+			dev_err(miic->dev, "Port number out of range: %d\n", port);
+			of_node_put(conv);
+			kfree(dt_val);
+			return -EINVAL;
+		}
+
 		if (of_property_read_u32(conv, "renesas,miic-input", &conf))
 			continue;
 

-- 
2.43.0

Re: [PATCH v4 2/2] net: pcs: rzn1-miic: Verify port number from dtb

[Andrew Lunn]

On Mon, Sep 14, 2026 at 09:41:01AM -0700, Kyle Hendry via B4 Relay wrote:
> From: Kyle Hendry <khendry@reliablecontrols.com>
> 
> Add check to make sure port number is in range before writing
> to dt_val array
> 
> Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v4 2/2] net: pcs: rzn1-miic: Verify port number from dtb

[netdev-bot+sashiko]

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

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

- [High] Incomplete fix — the new bounds check in miic_parse_dt()
  (drivers/net/pcs/pcs-rzn1-miic.c:696-702) uses `port >…
- [Medium] The commit message ("Add check to make sure port number is in
  range before writing to dt_val array") never states the pre-patch…

Pre-existing issues:
- [High] `renesas,miic-input` (`conf`) is stored into `dt_val[]` at
  drivers/net/pcs/pcs-rzn1-miic.c:708 (and `renesas,miic-switch-portin`…
- [Medium] `miic_port_max` is documented as "Maximum MIIC supported"
  (drivers/net/pcs/pcs-rzn1-miic.c:225) but is consumed with two…

--

net: pcs: rzn1-miic: Verify port number from dtb

This adds a range check on the converter child node "reg" value in
miic_parse_dt() before it is used as an index into the dt_val array, and
returns -EINVAL after releasing the node reference and freeing dt_val.

> net: pcs: rzn1-miic: Verify port number from dtb
>
>     Add check to make sure port number is in range before writing
>     to dt_val array

[Severity: Medium]
Could the changelog say what happens today without the check?  In the
parent commit, miic_parse_dt() takes "reg" straight from the device tree
and uses it as an index:

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;
		...
		dt_val[port + !miic->of_data->miic_port_start] = conf;
}

so an out-of-range reg is a slab out-of-bounds write during probe.  Should
this carry a Fixes: tag pointing at the commit that introduced the
unvalidated indexing (7dc54d3b8d9100, later widened by f39e968dc168a7 /
61f1139a476569) so stable can pick it up?  The preceding patch in this
series ("net: pcs: rzn1-miic: Fix config array initialization") does have
one.

It may also be worth mentioning the behaviour change: probe now fails and
the whole MII converter becomes unavailable for device trees that
previously probed with a bogus reg.

> diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
> index cb74861e823c8..1bd95ddf9400a 100644
> --- a/drivers/net/pcs/pcs-rzn1-miic.c
> +++ b/drivers/net/pcs/pcs-rzn1-miic.c
> @@ -693,6 +693,14 @@ 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->of_data->miic_port_max) {

[Severity: High]
Is the upper bound off by one for the RZ/T2H variant?  The comparison is
inclusive, but miic_port_max is used as a converter count elsewhere:

rzt2h_miic_of_data = {
	.conf_conv_count = 5,
	.miic_port_start = 0,
	.miic_port_max = 4,
};

miic_init_hw() treats it as an exclusive count:

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

So for compatible "renesas,r9a09g077-miic" a child with reg = <4> passes
the new test, and the store a few lines below becomes dt_val[5] on a
5-byte buffer:

	dt_val = kmalloc_objs(*dt_val, miic->of_data->conf_conv_count);
	...
	/* Adjust for 0 based index */
	dt_val[port + !miic->of_data->miic_port_start] = conf;

Does that still overflow dt_val[] by one byte with the DT-supplied
renesas,miic-input value?  The binding's non-rzn1 branch restricts
converter reg to "enum: [0, 1, 2, 3]", and
include/dt-bindings/net/renesas,r9a09g077-pcs-miic.h documents only
Port 0..Port 3, so reg = <4> looks like exactly the malformed input this
patch is meant to reject.

Would an index-derived predicate work better, e.g.

	if (port < miic->of_data->miic_port_start ||
	    port + !miic->of_data->miic_port_start >= miic->of_data->conf_conv_count)

RZ/N1 (miic_port_start = 1, miic_port_max = 5, conf_conv_count = 6) is
unaffected, which is probably why this is not visible on the common
platform.

There is a second effect of accepting port 4: miic_configure_phy_link()
does

	mask = BIT(port + shift);

and with an ETHSW input (shift 0) port 4 sets BIT(4), which is in the
ESC/EtherCAT nibble [7:4] of MIIC_PHY_LINK that miic_probe() later
programs via miic_reg_rmw(miic, MIIC_PHY_LINK, ...).  Can that clobber
EtherCAT port 0 link polarity?

[Severity: Medium]
This isn't a bug introduced by this patch, but miic_port_max now has three
consumers with two conflicting meanings.  The kernel-doc for struct
miic_of_data says "Maximum MIIC supported", miic_init_hw() uses it as an
exclusive count (port < miic_port_max), miic_create() uses it as an
inclusive maximum reg value:

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

and this patch adds a third site with the inclusive reading.  On
r9a09g077 the inclusive reading lets miic_create() hand out a PCS for
converter 4, which does not exist (rzt2h_index_to_string[] lists only
SWITCH_PORTIN and CONV0..CONV3), after which miic_config() /
miic_converter_enable() program MIIC_CONVCTRL(4) and MIIC_CONVRST bit 4.
Could the field be documented and used consistently, so that fixing one
site does not break the others?

> +			dev_err(miic->dev, "Port number out of range: %d\n", port);
> +			of_node_put(conv);
> +			kfree(dt_val);
> +			return -EINVAL;
> +		}
> +
>  		if (of_property_read_u32(conv, "renesas,miic-input", &conf))
>  			continue;
>  

[Severity: High]
This is a pre-existing issue, but since this block is now the validation
point for the child node, would it make sense to bound conf here too?
renesas,miic-input is stored without any range check against
conf_to_string_count, and with a silent u32 to s8 truncation:

	if (of_property_read_u32(np, "renesas,miic-switch-portin", &conf) == 0)
		dt_val[0] = conf;
	...
		dt_val[port + !miic->of_data->miic_port_start] = conf;

On a match failure, miic_match_dt_conf() -> miic_dump_conf() uses the
stored value as an index and then prints the result:

	if (conf[i] != MIIC_MODCTRL_CONF_NONE)
		conf_name = of_data->conf_to_string[conf[i]];
	...
	dev_err(miic->dev, "%s: %s\n", of_data->index_to_string[i], conf_name);

With renesas,miic-input = <100> nothing in modctrl_match_table[] matches,
so miic_dump_conf() reads past the 14-entry conf_to_string[] (9-entry
rzt2h_conf_to_string[]) and hands the resulting word to %s.  Can that
oops or leak kernel memory into dmesg?

Also, conf = 255 truncates to -1, which equals MIIC_MODCTRL_CONF_NONE, so
miic_modctrl_match() treats the entry as a wildcard instead of rejecting
it.  Is that intended?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914-rzn1-miic-fix-array-v4-0-069112feec86%40reliablecontrols.com