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

["Karumanchi, Vineeth" <vineeth@amd.com>]

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