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