| Message ID | 20240702110650.17563-1-amishin@t-argos.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: phy: mscc-miim: Validate bus frequency obtained from Device Tree | expand |
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;
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
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;
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(+)