Re: [PATCH 2/2] net: phy: dp83867: support setting output-impedance

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Inverted error check on `of_property_read_u32()` ignores the property when present and causes probe failure or garbage configuration when missing.
- [High] Passing a pointer to a 1-byte variable to a function expecting a 4-byte pointer causes a stack buffer overflow.
- [Medium] Meaningful code and commit message mismatch: nvmem takes highest precedence, not lowest.
--

--- Patch [2]: [PATCH 2/2] net: phy: dp83867: support setting output-impedance ---
commit 5d72fffedcf9773506f98861b65736e9f8be835a
Author: Steffen Trumtrar <[email protected]>

net: phy: dp83867: support setting output-impedance

This commit adds support for the ti,output-impedance devicetree binding to
configure the IO impedance of the DP83867 PHY. It introduces a fallback to
this new property when nvmem is unavailable.

> Currently the dp83867 supports setting the io impedance to minimum,
> maximum, default or a value from a nvmem cell. In situations where there
> is no backend for a nvmem cell, the value can not be set to anything but
> minimum, maximum or default.
> 
> Add support for the ti,output-impedance binding, allowing to specify the
> impedance via the devicetree. The existing boolean bindings for minimum
> and maximum take precedence over this new binding. The nvmem consumer
> still takes lowest precedence.

[Severity: Medium]
Is this commit message accurate regarding the precedence?

Looking at dp83867_of_init_io_impedance(), it seems the code attempts to
retrieve the nvmem cell first. If that succeeds, the early return prevents
any device tree fallback logic from running:

dp83867_of_init_io_impedance() {
	cell = of_nvmem_cell_get(of_node, "io_impedance_ctrl");
	if (IS_ERR(cell)) {
		// DT fallback logic happens here
		return 0;
	}
	...
}

This would give the nvmem consumer the highest precedence rather than the
lowest. Could the commit message be clarified?

> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c

[ ... ]

> @@ -542,12 +542,19 @@ static int dp83867_of_init_io_impedance(struct phy_device *phydev)
>  						"failed to get nvmem cell io_impedance_ctrl\n");
>  
>  		/* If no nvmem cell, check for the boolean properties. */
> -		if (of_property_read_bool(of_node, "ti,max-output-impedance"))
> +		if (of_property_read_bool(of_node, "ti,max-output-impedance")) {
>  			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MAX;
> -		else if (of_property_read_bool(of_node, "ti,min-output-impedance"))
> +		} else if (of_property_read_bool(of_node, "ti,min-output-impedance")) {
>  			dp83867->io_impedance = DP83867_IO_MUX_CFG_IO_IMPEDANCE_MIN;
> -		else
> +		} else if (of_property_read_u32(of_node, "ti,output-impedance", &val)) {

[Severity: High]
Does this condition check for success properly?

The function of_property_read_u32() returns 0 on success and a negative error
code on failure. Because of this, the if condition evaluates to true when the
property is missing (returning a negative value), and false when it is
successfully read (returning 0).

This means the driver might use an uninitialized stack value for configuration
and fail the probe if the property is absent, while ignoring the property
entirely if it is present.

[Severity: High]
Can this cause a stack buffer overflow?

The variable val is declared as a u8 earlier in dp83867_of_init_io_impedance():

dp83867_of_init_io_impedance() {
	u8 *buf, val;
	...
}

However, of_property_read_u32() expects a u32 pointer. Passing a pointer to
a 1-byte allocation into a function that writes 4 bytes could corrupt adjacent
stack memory.

> +			if ((val & DP83867_IO_MUX_CFG_IO_IMPEDANCE_MASK) != val) {
> +				phydev_err(phydev, "'ti,output-impedance' contents out of range\n");
> +				return -ERANGE;
> +			}
> +			dp83867->io_impedance = val;
> +		} else {
>  			dp83867->io_impedance = -1; /* leave at default */
> +		}
>  
>  		return 0;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2