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

[netdev-bot+sashiko@kernel.org]

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