Re: [PATCH net-next v2 3/3] 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

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 commit fixes a memory-safety bug but has no Fixes: tag.
- [Low] The new error message in miic_parse_dt() prints `conf`, declared
  `u32 conf;`, with `%d`: `dev_err(miic->dev, "Port %d configuration out…

--

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

This commit adds range checks in miic_parse_dt() for the per-port "reg"
and "renesas,miic-input" device tree values. 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 parsed values are within range for the SoC and fail the
> probe if invalid.

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

Before this patch, an unchecked "reg" value in miic_parse_dt() could
write past the end of the kmalloc'ed dt_val[] buffer. That buffer is only
conf_conv_count bytes:

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

The unchecked index seems to date back to the original driver. There,
dt_val[] was a stack array indexed directly by the DT "reg" value.
Would this trailer be appropriate?

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

Two earlier patches in this series already carry Fixes: tags: "net: pcs:
rzn1-miic: Fix port numbering on RZ/T2H" and "net: pcs: rzn1-miic: Fix
miic register initialization loop".

> diff --git a/drivers/net/pcs/pcs-rzn1-miic.c b/drivers/net/pcs/pcs-rzn1-miic.c
> index 9e830932ce49c..9a4c6d3b7d424 100644
> --- a/drivers/net/pcs/pcs-rzn1-miic.c
> +++ b/drivers/net/pcs/pcs-rzn1-miic.c
> @@ -697,9 +697,25 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
>  		if (of_property_read_u32(conv, "reg", &port))
>  			continue;
>  

[ ... ]

>  		if (of_property_read_u32(conv, "renesas,miic-input", &conf))
>  			continue;
>  
> +		if (conf >= miic->of_data->conf_to_string_count) {

[Severity: High]
This checks "renesas,miic-input", but what about the parent node's
"renesas,miic-switch-portin" value earlier in miic_parse_dt()? It is
still stored with no range check:

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

The u32 is truncated into an s8. An out-of-range value will not match
any entry in the match table. miic_match_dt_conf() then falls through to
miic_dump_conf(), which does:

	conf_name = of_data->conf_to_string[conf[i]];

Can this read a pointer from past the end of conf_to_string[] and then
pass it to dev_err() as a %s argument? Values from 128 to 254 become
negative s8 values, so for those the read would land before the start
of the array.

The binding allows only 1 or 2 on RZ/N1 and only 0 on RZ/T2H/N2H. A
value of 255 truncates to -1 (MIIC_MODCTRL_CONF_NONE) and is silently
treated as absent.

Could the same conf_to_string_count check, or the stricter per-SoC
binding values, be applied to switch-portin before it is stored in
dt_val[0]?

> +			dev_err(miic->dev, "Port %d configuration out of range: %d\n",
> +				port, conf);

[Severity: Low]
This isn't a bug, but conf is declared u32, so should this be %u? Any DT
value of 0x80000000 or above prints as negative. 0xffffffff prints as -1,
which looks like the MIIC_MODCTRL_CONF_NONE sentinel.

> +			of_node_put(conv);
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-miic-validate-dtb-v2-0-1ee3d33748e9%40reliablecontrols.com