[lvc-project] [PATCH net] net: phy: mscc-miim: Validate bus frequency obtained from Device Tree

[Andrew Lunn]

On Tue, Jul 02, 2024 at 02:06:50PM +0300, Aleksandr Mishin wrote:
> In mscc_miim_clk_set() miim->bus_freq is taken from Device Tree and can
> contain any value in case of any error or broken DT. A value of 2147483648
> multiplied by 2 will result in an overflow and division by 0.
> 
> Add bus frequency value check to avoid overflow.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: bb2a1934ca01 ("net: phy: mscc-miim: add support to set MDIO bus frequency")
> Signed-off-by: Aleksandr Mishin <amishin at t-argos.ru>
> ---
>  drivers/net/mdio/mdio-mscc-miim.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/mdio/mdio-mscc-miim.c b/drivers/net/mdio/mdio-mscc-miim.c
> index c29377c85307..6380c22567ea 100644
> --- a/drivers/net/mdio/mdio-mscc-miim.c
> +++ b/drivers/net/mdio/mdio-mscc-miim.c
> @@ -254,6 +254,11 @@ static int mscc_miim_clk_set(struct mii_bus *bus)
>  	if (!miim->bus_freq)
>  		return 0;
>  
> +	if (miim->bus_freq == 2147483648) {
> +		dev_err(&bus->dev, "Incorrect bus frequency\n");
> +		return -EINVAL;
> +	}

Are you saying that only this one specific value with cause overflow?
No other value will? Can any value cause underflow?

Generally, i would expect to see a range check here. 802.3 requires
that the bus can operate at 2.5MHz. I've seen some which can operate
up to 12Mhz. 25MHz seems like a sensible upper limit. But maybe you
can do the maths and figure out the theoretical maximum. There are use
cases for going below 2.5MHz, the hardware design is broken, the edge
on the signal are two round at 2.5Mhz. So i've seen prototype hardware
need to run there MDIO clock at 100Khz until the hardware designer
fixes their error. So how about validating the frequency is > 100KHz
and < 25MHz?

> +		dev_err(&bus->dev, "Incorrect bus frequency\n");

Incorrect is also a bit odd. I would prefer Invalid.

	Andrew