| Message ID | cdf664ea-3312-e915-73f8-021678d08887@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 867dbe784c5010a466f00a7d1467c1c5ea569c75 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: mdio: validate parameter addr in mdiobus_get_phy() | expand |
On Sun, Jan 15, 2023 at 11:54:06AM +0100, Heiner Kallweit wrote: > The caller may pass any value as addr, what may result in an out-of-bounds > access to array mdio_map. One existing case is stmmac_init_phy() that > may pass -1 as addr. Therefore validate addr before using it. > > Fixes: 7f854420fbfe ("phy: Add API for {un}registering an mdio device to a bus.") > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Hi Heiner This makes sense, but we should also fix stmmac to not actually do this, since it is clearly wrong, and probably indicates a configuration problem for that driver. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Sun, 2023-01-15 at 11:54 +0100, Heiner Kallweit wrote: > The caller may pass any value as addr, what may result in an out-of-bounds > access to array mdio_map. One existing case is stmmac_init_phy() that > may pass -1 as addr. Therefore validate addr before using it. > > Fixes: 7f854420fbfe ("phy: Add API for {un}registering an mdio device to a bus.") > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/net/phy/mdio_bus.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > index 902e1c88e..132dd1f90 100644 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c > @@ -108,7 +108,12 @@ EXPORT_SYMBOL(mdiobus_unregister_device); > > struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr) > { > - struct mdio_device *mdiodev = bus->mdio_map[addr]; > + struct mdio_device *mdiodev; > + > + if (addr < 0 || addr >= ARRAY_SIZE(bus->mdio_map)) > + return NULL; Speaking of possible follow-ups, would it make sense to add a WARN_ON_ONCE() or similar on the above condition? Thanks! Paolo >
Hello: This patch was applied to netdev/net.git (master) by Paolo Abeni <pabeni@redhat.com>: On Sun, 15 Jan 2023 11:54:06 +0100 you wrote: > The caller may pass any value as addr, what may result in an out-of-bounds > access to array mdio_map. One existing case is stmmac_init_phy() that > may pass -1 as addr. Therefore validate addr before using it. > > Fixes: 7f854420fbfe ("phy: Add API for {un}registering an mdio device to a bus.") > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > [...] Here is the summary with links: - [net] net: mdio: validate parameter addr in mdiobus_get_phy() https://git.kernel.org/netdev/net/c/867dbe784c50 You are awesome, thank you!
On 17.01.2023 12:31, Paolo Abeni wrote: > On Sun, 2023-01-15 at 11:54 +0100, Heiner Kallweit wrote: >> The caller may pass any value as addr, what may result in an out-of-bounds >> access to array mdio_map. One existing case is stmmac_init_phy() that >> may pass -1 as addr. Therefore validate addr before using it. >> >> Fixes: 7f854420fbfe ("phy: Add API for {un}registering an mdio device to a bus.") >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/net/phy/mdio_bus.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c >> index 902e1c88e..132dd1f90 100644 >> --- a/drivers/net/phy/mdio_bus.c >> +++ b/drivers/net/phy/mdio_bus.c >> @@ -108,7 +108,12 @@ EXPORT_SYMBOL(mdiobus_unregister_device); >> >> struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr) >> { >> - struct mdio_device *mdiodev = bus->mdio_map[addr]; >> + struct mdio_device *mdiodev; >> + >> + if (addr < 0 || addr >= ARRAY_SIZE(bus->mdio_map)) >> + return NULL; > > Speaking of possible follow-ups, would it make sense to add a > WARN_ON_ONCE() or similar on the above condition? > Yes, I think that's a good idea. I'll send a follow-up patch. > Thanks! > > Paolo >> >
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 902e1c88e..132dd1f90 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -108,7 +108,12 @@ EXPORT_SYMBOL(mdiobus_unregister_device); struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr) { - struct mdio_device *mdiodev = bus->mdio_map[addr]; + struct mdio_device *mdiodev; + + if (addr < 0 || addr >= ARRAY_SIZE(bus->mdio_map)) + return NULL; + + mdiodev = bus->mdio_map[addr]; if (!mdiodev) return NULL;
The caller may pass any value as addr, what may result in an out-of-bounds access to array mdio_map. One existing case is stmmac_init_phy() that may pass -1 as addr. Therefore validate addr before using it. Fixes: 7f854420fbfe ("phy: Add API for {un}registering an mdio device to a bus.") Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/mdio_bus.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)