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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ed9517b1-0945-4a43-b4b0-dce942b05889@lunn.ch>
Date: Tue, 2 Jul 2024 15:36:29 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Aleksandr Mishin <amishin@...rgos.ru>
Cc: Michael Walle <michael@...le.cc>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	lvc-project@...uxtesting.org
Subject: Re: [PATCH net] net: phy: mscc-miim: Validate bus frequency obtained
 from Device Tree

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@...rgos.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