[PATCH net] net: macb: fix NULL pointer dereference on unbind with fixed-link

[PATCH net] net: macb: fix NULL pointer dereference on unbind with fixed-link

[Vineeth Karumanchi]

When the device tree describes a fixed-link and has no "mdio" child
node, macb_mii_init() returns early without allocating the MDIO bus,
leaving bp->mii_bus as NULL.

Two cleanup paths then dereference this NULL bus:

1. On driver unbind, macb_remove() unconditionally calls
   mdiobus_unregister(bp->mii_bus), which oopses:

  Unable to handle kernel NULL pointer dereference at virtual address 00000000000004a8
  pc : mdiobus_unregister+0x14/0xa4
  lr : macb_remove+0x38/0xa4
  Call trace:
   mdiobus_unregister+0x14/0xa4 (P)
   macb_remove+0x38/0xa4
   platform_remove+0x20/0x30
   device_release_driver_internal+0x1c8/0x224
   unbind_store+0xb4/0xbc

2. On the probe error path in macb_probe(), reached when
   macb_mii_init() has succeeded but a subsequent step fails, the
   err_out_unregister_mdio label runs the same unconditional cleanup.

mdiobus_unregister() and mdiobus_free() do not guard against a NULL
bus, so guard the calls in both macb_remove() and the probe error
path.

Fixes: d0c3601f2c4e ("net: macb: Avoid 20s boot delay by skipping MDIO bus registration for fixed-link PHY")
Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 76ee4f506033..88eebe187eac 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5971,8 +5971,10 @@ static int macb_probe(struct platform_device *pdev)
 	macb_free_tieoff(bp);
 
 err_out_unregister_mdio:
-	mdiobus_unregister(bp->mii_bus);
-	mdiobus_free(bp->mii_bus);
+	if (bp->mii_bus) {
+		mdiobus_unregister(bp->mii_bus);
+		mdiobus_free(bp->mii_bus);
+	}
 
 err_out_phy_exit:
 	phy_exit(bp->phy);
@@ -6001,8 +6003,10 @@ static void macb_remove(struct platform_device *pdev)
 		unregister_netdev(netdev);
 		macb_free_tieoff(bp);
 		phy_exit(bp->phy);
-		mdiobus_unregister(bp->mii_bus);
-		mdiobus_free(bp->mii_bus);
+		if (bp->mii_bus) {
+			mdiobus_unregister(bp->mii_bus);
+			mdiobus_free(bp->mii_bus);
+		}
 
 		device_set_wakeup_enable(&bp->pdev->dev, 0);
 		cancel_delayed_work_sync(&bp->tx_lpi_work);
-- 
2.43.0

Re: [PATCH net] net: macb: fix NULL pointer dereference on unbind with fixed-link

[Xuanqiang Luo]

在 2026/9/2 18:28, Vineeth Karumanchi 写道:
> When the device tree describes a fixed-link and has no "mdio" child
> node, macb_mii_init() returns early without allocating the MDIO bus,
> leaving bp->mii_bus as NULL.
>
> Two cleanup paths then dereference this NULL bus:
>
> 1. On driver unbind, macb_remove() unconditionally calls
>     mdiobus_unregister(bp->mii_bus), which oopses:
>
>    Unable to handle kernel NULL pointer dereference at virtual address 00000000000004a8
>    pc : mdiobus_unregister+0x14/0xa4
>    lr : macb_remove+0x38/0xa4
>    Call trace:
>     mdiobus_unregister+0x14/0xa4 (P)
>     macb_remove+0x38/0xa4
>     platform_remove+0x20/0x30
>     device_release_driver_internal+0x1c8/0x224
>     unbind_store+0xb4/0xbc
>
> 2. On the probe error path in macb_probe(), reached when
>     macb_mii_init() has succeeded but a subsequent step fails, the
>     err_out_unregister_mdio label runs the same unconditional cleanup.
>
> mdiobus_unregister() and mdiobus_free() do not guard against a NULL
> bus, so guard the calls in both macb_remove() and the probe error
> path.
>
> Fixes: d0c3601f2c4e ("net: macb: Avoid 20s boot delay by skipping MDIO bus registration for fixed-link PHY")
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>

Reviewed-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Thanks,
Xuanqiang

Re: [PATCH net] net: macb: fix NULL pointer dereference on unbind with fixed-link

[Nicolai Buchwitz]

Hi Vineeth

On 2.9.2026 12:28, Vineeth Karumanchi wrote:
> When the device tree describes a fixed-link and has no "mdio" child
> node, macb_mii_init() returns early without allocating the MDIO bus,
> leaving bp->mii_bus as NULL.
> 
> Two cleanup paths then dereference this NULL bus:
> 
> 1. On driver unbind, macb_remove() unconditionally calls
>    mdiobus_unregister(bp->mii_bus), which oopses:
> 
>   Unable to handle kernel NULL pointer dereference at virtual address 
> 00000000000004a8
>   pc : mdiobus_unregister+0x14/0xa4
>   lr : macb_remove+0x38/0xa4
>   Call trace:
>    mdiobus_unregister+0x14/0xa4 (P)
>    macb_remove+0x38/0xa4
>    platform_remove+0x20/0x30
>    device_release_driver_internal+0x1c8/0x224
>    unbind_store+0xb4/0xbc
> 
> 2. On the probe error path in macb_probe(), reached when
>    macb_mii_init() has succeeded but a subsequent step fails, the
>    err_out_unregister_mdio label runs the same unconditional cleanup.
> 
> mdiobus_unregister() and mdiobus_free() do not guard against a NULL
> bus, so guard the calls in both macb_remove() and the probe error
> path.
> 
> Fixes: d0c3601f2c4e ("net: macb: Avoid 20s boot delay by skipping MDIO 
> bus registration for fixed-link PHY")
> Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
> ---
>  drivers/net/ethernet/cadence/macb_main.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
> b/drivers/net/ethernet/cadence/macb_main.c
> index 76ee4f506033..88eebe187eac 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -5971,8 +5971,10 @@ static int macb_probe(struct platform_device 
> *pdev)
>  	macb_free_tieoff(bp);
> 
>  err_out_unregister_mdio:
> -	mdiobus_unregister(bp->mii_bus);
> -	mdiobus_free(bp->mii_bus);
> +	if (bp->mii_bus) {
> +		mdiobus_unregister(bp->mii_bus);
> +		mdiobus_free(bp->mii_bus);
> +	}
> 
>  err_out_phy_exit:
>  	phy_exit(bp->phy);
> @@ -6001,8 +6003,10 @@ static void macb_remove(struct platform_device 
> *pdev)
>  		unregister_netdev(netdev);
>  		macb_free_tieoff(bp);
>  		phy_exit(bp->phy);
> -		mdiobus_unregister(bp->mii_bus);
> -		mdiobus_free(bp->mii_bus);
> +		if (bp->mii_bus) {
> +			mdiobus_unregister(bp->mii_bus);
> +			mdiobus_free(bp->mii_bus);
> +		}

nit: replica of the code from above, but also not really worth a helper?

> 
>  		device_set_wakeup_enable(&bp->pdev->dev, 0);
>  		cancel_delayed_work_sync(&bp->tx_lpi_work);

Anyway:

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net] net: macb: fix NULL pointer dereference on unbind with fixed-link

[Théo Lebrun]

Hello Vineeth,

On Wed Sep 2, 2026 at 12:28 PM CEST, Vineeth Karumanchi wrote:
> When the device tree describes a fixed-link and has no "mdio" child
> node, macb_mii_init() returns early without allocating the MDIO bus,
> leaving bp->mii_bus as NULL.
>
> Two cleanup paths then dereference this NULL bus:
>
> 1. On driver unbind, macb_remove() unconditionally calls
>    mdiobus_unregister(bp->mii_bus), which oopses:
>
>   Unable to handle kernel NULL pointer dereference at virtual address 00000000000004a8
>   pc : mdiobus_unregister+0x14/0xa4
>   lr : macb_remove+0x38/0xa4
>   Call trace:
>    mdiobus_unregister+0x14/0xa4 (P)
>    macb_remove+0x38/0xa4
>    platform_remove+0x20/0x30
>    device_release_driver_internal+0x1c8/0x224
>    unbind_store+0xb4/0xbc
>
> 2. On the probe error path in macb_probe(), reached when
>    macb_mii_init() has succeeded but a subsequent step fails, the
>    err_out_unregister_mdio label runs the same unconditional cleanup.
>
> mdiobus_unregister() and mdiobus_free() do not guard against a NULL
> bus, so guard the calls in both macb_remove() and the probe error
> path.

Agreed on the patch! However as we are there, I looked at all
bp->mii_bus usage and macb_phylink_connect() might crash if bp->mii_bus
is NULL.

bp->mii_bus is NULL if
 - no children mdio DT node AND
 - DT declared fixed-link, see of_phy_is_fixed_link()

In macb_phylink_connect(), phy_find_first(bp->mii_bus) will be called if
 - phylink_of_phy_connect() fails AND
 - we have no phy-handle phandle prop

Those two overlap right?

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net] net: macb: fix NULL pointer dereference on unbind with fixed-link

[Théo Lebrun]

Hi Nicolai,

On Thu Sep 3, 2026 at 9:23 AM CEST, Nicolai Buchwitz wrote:
> On 2.9.2026 12:28, Vineeth Karumanchi wrote:
>> @@ -5971,8 +5971,10 @@ static int macb_probe(struct platform_device 
>> *pdev)
>>  	macb_free_tieoff(bp);
>> 
>>  err_out_unregister_mdio:
>> -	mdiobus_unregister(bp->mii_bus);
>> -	mdiobus_free(bp->mii_bus);
>> +	if (bp->mii_bus) {
>> +		mdiobus_unregister(bp->mii_bus);
>> +		mdiobus_free(bp->mii_bus);
>> +	}
>> 
>>  err_out_phy_exit:
>>  	phy_exit(bp->phy);
>> @@ -6001,8 +6003,10 @@ static void macb_remove(struct platform_device 
>> *pdev)
>>  		unregister_netdev(netdev);
>>  		macb_free_tieoff(bp);
>>  		phy_exit(bp->phy);
>> -		mdiobus_unregister(bp->mii_bus);
>> -		mdiobus_free(bp->mii_bus);
>> +		if (bp->mii_bus) {
>> +			mdiobus_unregister(bp->mii_bus);
>> +			mdiobus_free(bp->mii_bus);
>> +		}
>
> nit: replica of the code from above, but also not really worth a helper?

Agreed that the helper is overblown!

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net] net: macb: fix NULL pointer dereference on unbind with fixed-link

[Karumanchi, Vineeth]

Hi Théo Lebrun,

On 9/3/2026 1:57 PM, Théo Lebrun wrote:
> Hello Vineeth,
> 
> On Wed Sep 2, 2026 at 12:28 PM CEST, Vineeth Karumanchi wrote:
>> When the device tree describes a fixed-link and has no "mdio" child
>> node, macb_mii_init() returns early without allocating the MDIO bus,
>> leaving bp->mii_bus as NULL.
>>
>> Two cleanup paths then dereference this NULL bus:
>>
>> 1. On driver unbind, macb_remove() unconditionally calls
>>    mdiobus_unregister(bp->mii_bus), which oopses:
>>
>>   Unable to handle kernel NULL pointer dereference at virtual address 00000000000004a8
>>   pc : mdiobus_unregister+0x14/0xa4
>>   lr : macb_remove+0x38/0xa4
>>   Call trace:
>>    mdiobus_unregister+0x14/0xa4 (P)
>>    macb_remove+0x38/0xa4
>>    platform_remove+0x20/0x30
>>    device_release_driver_internal+0x1c8/0x224
>>    unbind_store+0xb4/0xbc
>>
>> 2. On the probe error path in macb_probe(), reached when
>>    macb_mii_init() has succeeded but a subsequent step fails, the
>>    err_out_unregister_mdio label runs the same unconditional cleanup.
>>
>> mdiobus_unregister() and mdiobus_free() do not guard against a NULL
>> bus, so guard the calls in both macb_remove() and the probe error
>> path.
> 
> Agreed on the patch! However as we are there, I looked at all
> bp->mii_bus usage and macb_phylink_connect() might crash if bp->mii_bus
> is NULL.
> 
> bp->mii_bus is NULL if
>  - no children mdio DT node AND
>  - DT declared fixed-link, see of_phy_is_fixed_link()
> 
> In macb_phylink_connect(), phy_find_first(bp->mii_bus) will be called if
>  - phylink_of_phy_connect() fails AND
>  - we have no phy-handle phandle prop
> 
> Those two overlap right?
> 

We did not observe crash in the scenario described above.
The AI-generated response below is also consistent with our observations:

A fixed-link forces phylink_of_phy_connect() to return 0 (success), so
the phy_find_first() branch is never entered.

So in macb_phylink_connect(), the fixed-link case gives dn != NULL and
ret == 0:
if (!dn || (ret && !macb_phy_handle_exists(dn))) {   /* false || (0 &&
…) → false */
        phydev = phy_find_first(bp->mii_bus);        /* not reached */

Both disjuncts are false, so phy_find_first(bp->mii_bus) is skipped.

Thanks,

> Thanks,
> 
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

-- 
🙏 Vineeth