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

[netdev-bot+sashiko@kernel.org]

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