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@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;
+	}
+
 	rate = clk_get_rate(miim->clk);
 
 	div = DIV_ROUND_UP(rate, 2 * miim->bus_freq) - 1;
-- 
2.30.2

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

Hi,

On Tue Jul 2, 2024 at 1:06 PM CEST, 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@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) {

Please avoid magic numbers.

Instead of this, can we reorder the code and detect whether
2*bus_freq will overflow?

-michael

> +		dev_err(&bus->dev, "Incorrect bus frequency\n");
> +		return -EINVAL;
> +	}
> +
>  	rate = clk_get_rate(miim->clk);
>  
>  	div = DIV_ROUND_UP(rate, 2 * miim->bus_freq) - 1;

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")
Suggested-by: Michael Walle <michael@walle.cc>
Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
---
v1->v2: Detect overflow event instead of using magic numbers as suggested by Michael

 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..b344d0b03d8e 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 (2 * miim->bus_freq == 0) {
+		dev_err(&bus->dev, "Incorrect bus frequency\n");
+		return -EINVAL;
+	}
+
 	rate = clk_get_rate(miim->clk);
 
 	div = DIV_ROUND_UP(rate, 2 * miim->bus_freq) - 1;
-- 
2.30.2

Hi,

Please post a new version of a patch at the earliest after 24 hours,
so reviewers got a chance to reply.

On Tue Jul 2, 2024 at 3:02 PM CEST, 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")
> Suggested-by: Michael Walle <michael@walle.cc>
> Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru>
> ---
> v1->v2: Detect overflow event instead of using magic numbers as suggested by Michael
>
>  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..b344d0b03d8e 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 (2 * miim->bus_freq == 0) {
> +		dev_err(&bus->dev, "Incorrect bus frequency\n");
> +		return -EINVAL;
> +	}

DRY. Save it in a variable and use it in DIV_ROUND_UP(). Also you
just check for the div-by-zero, but what if the value will overflow
beyond zero? The calculation will be wrong, right?

There's also check_mul_overflow() but not sure that's encouraged in
netdev.

-michael

> +
>  	rate = clk_get_rate(miim->clk);
>  
>  	div = DIV_ROUND_UP(rate, 2 * miim->bus_freq) - 1;

On Tue, Jul 02, 2024 at 03:16:21PM +0200, Michael Walle wrote:
> Hi,
> 
> Please post a new version of a patch at the earliest after 24 hours,
> so reviewers got a chance to reply.

Just adding to that, please also read:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

	Andrew