[PATCH] net/mdiobus: Fix potential out-of-bounds read/write access

[PATCH] net/mdiobus: Fix potential out-of-bounds read/write access

[Jakub Raczynski]

When using publicly available tools like 'mdio-tools' to read/write data
from/to network interface and its PHY via mdiobus, there is no verification of
parameters passed to the ioctl and it accepts any mdio address.
Currently there is support for 32 addresses in kernel via PHY_MAX_ADDR define,
but it is possible to pass higher value than that via ioctl.
While read/write operation should generally fail in this case,
mdiobus provides stats array, where wrong address may allow out-of-bounds
read/write.

Fix that by adding address verification before read/write operation.
While this excludes this access from any statistics, it improves security of
read/write operation.

Fixes: 080bb352fad00 ("net: phy: Maintain MDIO device and bus statistics")
Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
Reported-by: Wenjing Shan <wenjing.shan@samsung.com>
---
 drivers/net/phy/mdio_bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index a6bcb0fee863..60fd0cd7cb9c 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -445,6 +445,9 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
 
 	lockdep_assert_held_once(&bus->mdio_lock);
 
+	if (addr >= PHY_MAX_ADDR)
+		return -ENXIO;
+
 	if (bus->read)
 		retval = bus->read(bus, addr, regnum);
 	else
@@ -474,6 +477,9 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 
 	lockdep_assert_held_once(&bus->mdio_lock);
 
+	if (addr >= PHY_MAX_ADDR)
+		return -ENXIO;
+
 	if (bus->write)
 		err = bus->write(bus, addr, regnum, val);
 	else
-- 
2.34.1

Re: [PATCH] net/mdiobus: Fix potential out-of-bounds read/write access

[Russell King (Oracle)]

On Mon, Jun 09, 2025 at 04:37:58PM +0200, Jakub Raczynski wrote:
> When using publicly available tools like 'mdio-tools' to read/write data
> from/to network interface and its PHY via mdiobus, there is no verification of
> parameters passed to the ioctl and it accepts any mdio address.
> Currently there is support for 32 addresses in kernel via PHY_MAX_ADDR define,
> but it is possible to pass higher value than that via ioctl.
> While read/write operation should generally fail in this case,
> mdiobus provides stats array, where wrong address may allow out-of-bounds
> read/write.
> 
> Fix that by adding address verification before read/write operation.
> While this excludes this access from any statistics, it improves security of
> read/write operation.
> 
> Fixes: 080bb352fad00 ("net: phy: Maintain MDIO device and bus statistics")
> Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
> Reported-by: Wenjing Shan <wenjing.shan@samsung.com>

This is insufficient on its own. If you check the clause 45 accessors,
they have the same issue, so this should also be fixed.

Your patch would've been fine for the blamed commit, but we've had
4e4aafcddbbf ("net: mdio: Add dedicated C45 API to MDIO bus drivers")
in v6.3.

For easier back-porting, it probably makes sense to have this patch
and another separate patch addressing the ones introduced in the
more recent commit - and the two patches sent as a patch series.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH 1/2] net/mdiobus: Fix potential out-of-bounds read/write access

[Jakub Raczynski]

When using publicly available tools like 'mdio-tools' to read/write data
from/to network interface and its PHY via mdiobus, there is no verification of
parameters passed to the ioctl and it accepts any mdio address.
Currently there is support for 32 addresses in kernel via PHY_MAX_ADDR define,
but it is possible to pass higher value than that via ioctl.
While read/write operation should generally fail in this case,
mdiobus provides stats array, where wrong address may allow out-of-bounds
read/write.

Fix that by adding address verification before read/write operation.
While this excludes this access from any statistics, it improves security of
read/write operation.

Fixes: 080bb352fad00 ("net: phy: Maintain MDIO device and bus statistics")
Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
Reported-by: Wenjing Shan <wenjing.shan@samsung.com>
---
 drivers/net/phy/mdio_bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index a6bcb0fee863..60fd0cd7cb9c 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -445,6 +445,9 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
 
 	lockdep_assert_held_once(&bus->mdio_lock);
 
+	if (addr >= PHY_MAX_ADDR)
+		return -ENXIO;
+
 	if (bus->read)
 		retval = bus->read(bus, addr, regnum);
 	else
@@ -474,6 +477,9 @@ int __mdiobus_write(struct mii_bus *bus, int addr, u32 regnum, u16 val)
 
 	lockdep_assert_held_once(&bus->mdio_lock);
 
+	if (addr >= PHY_MAX_ADDR)
+		return -ENXIO;
+
 	if (bus->write)
 		err = bus->write(bus, addr, regnum, val);
 	else
-- 
2.34.1

[PATCH 2/2] net/mdiobus: Fix potential out-of-bounds clause 45 read/write access

[Jakub Raczynski]

When using publicly available tools like 'mdio-tools' to read/write data
from/to network interface and its PHY via C45 (clause 45) mdiobus,
there is no verification of parameters passed to the ioctl and
it accepts any mdio address.
Currently there is support for 32 addresses in kernel via PHY_MAX_ADDR define,
but it is possible to pass higher value than that via ioctl.
While read/write operation should generally fail in this case,
mdiobus provides stats array, where wrong address may allow out-of-bounds
read/write.

Fix that by adding address verification before C45 read/write operation.
While this excludes this access from any statistics, it improves security of
read/write operation.

Fixes: 4e4aafcddbbf ("net: mdio: Add dedicated C45 API to MDIO bus drivers")
Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
Reported-by: Wenjing Shan <wenjing.shan@samsung.com>
---
 drivers/net/phy/mdio_bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 60fd0cd7cb9c..fda2e27c1810 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -541,6 +541,9 @@ int __mdiobus_c45_read(struct mii_bus *bus, int addr, int devad, u32 regnum)
 
 	lockdep_assert_held_once(&bus->mdio_lock);
 
+	if (addr >= PHY_MAX_ADDR)
+		return -ENXIO;
+
 	if (bus->read_c45)
 		retval = bus->read_c45(bus, addr, devad, regnum);
 	else
@@ -572,6 +575,9 @@ int __mdiobus_c45_write(struct mii_bus *bus, int addr, int devad, u32 regnum,
 
 	lockdep_assert_held_once(&bus->mdio_lock);
 
+	if (addr >= PHY_MAX_ADDR)
+		return -ENXIO;
+
 	if (bus->write_c45)
 		err = bus->write_c45(bus, addr, devad, regnum, val);
 	else
-- 
2.34.1

Re: [PATCH 1/2] net/mdiobus: Fix potential out-of-bounds read/write access

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Mon,  9 Jun 2025 17:31:46 +0200 you wrote:
> When using publicly available tools like 'mdio-tools' to read/write data
> from/to network interface and its PHY via mdiobus, there is no verification of
> parameters passed to the ioctl and it accepts any mdio address.
> Currently there is support for 32 addresses in kernel via PHY_MAX_ADDR define,
> but it is possible to pass higher value than that via ioctl.
> While read/write operation should generally fail in this case,
> mdiobus provides stats array, where wrong address may allow out-of-bounds
> read/write.
> 
> [...]

Here is the summary with links:
  - [1/2] net/mdiobus: Fix potential out-of-bounds read/write access
    https://git.kernel.org/netdev/net/c/0e629694126c
  - [2/2] net/mdiobus: Fix potential out-of-bounds clause 45 read/write access
    https://git.kernel.org/netdev/net/c/260388f79e94

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH 1/2] net/mdiobus: Fix potential out-of-bounds read/write access

[Dan Carpenter]

On Mon, Jun 09, 2025 at 05:31:46PM +0200, Jakub Raczynski wrote:
> When using publicly available tools like 'mdio-tools' to read/write data
> from/to network interface and its PHY via mdiobus, there is no verification of
> parameters passed to the ioctl and it accepts any mdio address.
> Currently there is support for 32 addresses in kernel via PHY_MAX_ADDR define,
> but it is possible to pass higher value than that via ioctl.
> While read/write operation should generally fail in this case,
> mdiobus provides stats array, where wrong address may allow out-of-bounds
> read/write.
> 
> Fix that by adding address verification before read/write operation.
> While this excludes this access from any statistics, it improves security of
> read/write operation.
> 
> Fixes: 080bb352fad00 ("net: phy: Maintain MDIO device and bus statistics")
> Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
> Reported-by: Wenjing Shan <wenjing.shan@samsung.com>
> ---
>  drivers/net/phy/mdio_bus.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> index a6bcb0fee863..60fd0cd7cb9c 100644
> --- a/drivers/net/phy/mdio_bus.c
> +++ b/drivers/net/phy/mdio_bus.c
> @@ -445,6 +445,9 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
>  
>  	lockdep_assert_held_once(&bus->mdio_lock);
>  
> +	if (addr >= PHY_MAX_ADDR)
> +		return -ENXIO;

addr is an int so Smatch wants this to be:

	if (addr < 0 || addr >= PHY_MAX_ADDR)
		return return -ENXIO;

I think that although addr is an int, the actual values are limited to
0-U16_MAX?

regards,
dan carpener

Re: [PATCH 1/2] net/mdiobus: Fix potential out-of-bounds read/write access

[Andrew Lunn]

On Wed, Jun 25, 2025 at 10:23:17AM -0500, Dan Carpenter wrote:
> On Mon, Jun 09, 2025 at 05:31:46PM +0200, Jakub Raczynski wrote:
> > When using publicly available tools like 'mdio-tools' to read/write data
> > from/to network interface and its PHY via mdiobus, there is no verification of
> > parameters passed to the ioctl and it accepts any mdio address.
> > Currently there is support for 32 addresses in kernel via PHY_MAX_ADDR define,
> > but it is possible to pass higher value than that via ioctl.
> > While read/write operation should generally fail in this case,
> > mdiobus provides stats array, where wrong address may allow out-of-bounds
> > read/write.
> > 
> > Fix that by adding address verification before read/write operation.
> > While this excludes this access from any statistics, it improves security of
> > read/write operation.
> > 
> > Fixes: 080bb352fad00 ("net: phy: Maintain MDIO device and bus statistics")
> > Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
> > Reported-by: Wenjing Shan <wenjing.shan@samsung.com>
> > ---
> >  drivers/net/phy/mdio_bus.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index a6bcb0fee863..60fd0cd7cb9c 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -445,6 +445,9 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
> >  
> >  	lockdep_assert_held_once(&bus->mdio_lock);
> >  
> > +	if (addr >= PHY_MAX_ADDR)
> > +		return -ENXIO;
> 
> addr is an int so Smatch wants this to be:
> 
> 	if (addr < 0 || addr >= PHY_MAX_ADDR)
> 		return return -ENXIO;

Yes, addr should never be negative.

> I think that although addr is an int, the actual values are limited to
> 0-U16_MAX?

No, addr should be in the range 0-31. regnum should also be in the
range 0-31. These are clause 22 accesses. There are also clause 45
accesses, but they don't come through here. For those, addr is still
in the range 0-31, but regnum is 0-U16_MAX.

	Andrew

Re: [PATCH 1/2] net/mdiobus: Fix potential out-of-bounds read/write access

[Russell King (Oracle)]

On Wed, Jun 25, 2025 at 10:23:17AM -0500, Dan Carpenter wrote:
> On Mon, Jun 09, 2025 at 05:31:46PM +0200, Jakub Raczynski wrote:
> > When using publicly available tools like 'mdio-tools' to read/write data
> > from/to network interface and its PHY via mdiobus, there is no verification of
> > parameters passed to the ioctl and it accepts any mdio address.
> > Currently there is support for 32 addresses in kernel via PHY_MAX_ADDR define,
> > but it is possible to pass higher value than that via ioctl.
> > While read/write operation should generally fail in this case,
> > mdiobus provides stats array, where wrong address may allow out-of-bounds
> > read/write.
> > 
> > Fix that by adding address verification before read/write operation.
> > While this excludes this access from any statistics, it improves security of
> > read/write operation.
> > 
> > Fixes: 080bb352fad00 ("net: phy: Maintain MDIO device and bus statistics")
> > Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
> > Reported-by: Wenjing Shan <wenjing.shan@samsung.com>
> > ---
> >  drivers/net/phy/mdio_bus.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > index a6bcb0fee863..60fd0cd7cb9c 100644
> > --- a/drivers/net/phy/mdio_bus.c
> > +++ b/drivers/net/phy/mdio_bus.c
> > @@ -445,6 +445,9 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
> >  
> >  	lockdep_assert_held_once(&bus->mdio_lock);
> >  
> > +	if (addr >= PHY_MAX_ADDR)
> > +		return -ENXIO;
> 
> addr is an int so Smatch wants this to be:
> 
> 	if (addr < 0 || addr >= PHY_MAX_ADDR)
> 		return return -ENXIO;
> 
> I think that although addr is an int, the actual values are limited to
> 0-U16_MAX?

0 to 31 inclusive.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/2] net/mdiobus: Fix potential out-of-bounds read/write access

[Jakub Raczynski]

On Wed, Jun 25, 2025 at 10:23:17AM -0500, Dan Carpenter wrote:
> On Mon, Jun 09, 2025 at 05:31:46PM +0200, Jakub Raczynski wrote:
> > > When using publicly available tools like 'mdio-tools' to read/write data
> > > from/to network interface and its PHY via mdiobus, there is no verification of
> > > parameters passed to the ioctl and it accepts any mdio address.
> > > Currently there is support for 32 addresses in kernel via PHY_MAX_ADDR define,
> > > but it is possible to pass higher value than that via ioctl.
> > > While read/write operation should generally fail in this case,
> > > mdiobus provides stats array, where wrong address may allow out-of-bounds
> > > read/write.
> > >
> > > Fix that by adding address verification before read/write operation.
> > > While this excludes this access from any statistics, it improves security of
> > > read/write operation.
> > >
> > > Fixes: 080bb352fad00 ("net: phy: Maintain MDIO device and bus statistics")
> > > Signed-off-by: Jakub Raczynski <j.raczynski@samsung.com>
> > > Reported-by: Wenjing Shan <wenjing.shan@samsung.com>
> > > ---
> > >  drivers/net/phy/mdio_bus.c 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
> > > index a6bcb0fee863..60fd0cd7cb9c 100644
> > > --- a/drivers/net/phy/mdio_bus.c
> > > +++ b/drivers/net/phy/mdio_bus.c
> > > @@ -445,6 +445,9 @@ int __mdiobus_read(struct mii_bus *bus, int addr, u32 regnum)
> > > 
> > >         lockdep_assert_held_once(&bus->mdio_lock);
> > > 
> > > +        if (addr >= PHY_MAX_ADDR)
> > > +                return -ENXIO;
> >
> > addr is an int so Smatch wants this to be:
> >
> >         if (addr < 0 addr >= PHY_MAX_ADDR)
> >                 return return -ENXIO;
> >
> > I think that although addr is an int, the actual values are limited to
> > 0-U16_MAX?

> 0 to 31 inclusive.

Should not be an issue. User calls use struct mii_ioctl_data which forces type of u16 so should not be a case. And some drivers force it too. Probably proper fix would be converting addr parameter to u16 to have consistent API.