Re: [PATCH net-next v14 04/13] net: phy: add phy_detach_internal() helper

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-after-free of MDIO bus when detaching a PHY.
- [High] Error path in `phy_attach_direct` catastrophically destroys existing PHY attachment and leaks references.
--

--- Patch [4]: [PATCH net-next v14 04/13] net: phy: add phy_detach_internal() helper ---
commit cd30833ec89f19b0a5658d942adf201f6894916e
Author: Markus Stockhausen <[email protected]>

net: phy: add phy_detach_internal() helper

For the upcoming MDIO bus notification enhancements the PHY detach path
needs to inform the bus about these actions. Until now phy_detach() is
called normally but also in the phy_direct_attch() cleanup parts. So in
the future there must be a clear indication if a notification is needed
(normal path) or not (cleanup path).

Carve out the phy_detach() code into a new helper phy_detach_internal()
and make phy_detach() a short stub that calls the new helper.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 94b2e85e00a37..835d71306b4d0 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1734,6 +1734,91 @@ static bool phy_drv_supports_irq(const struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->handle_interrupt;
>  }
>  
> +static void phy_detach_internal(struct phy_device *phydev)
> +{
> +	struct net_device *dev = phydev->attached_dev;
> +	struct module *ndev_owner = NULL;
> +	struct mii_bus *bus;
> +

[ ... ]

> +	phy_suspend(phydev);
> +	if (dev) {
> +		struct hwtstamp_provider *hwprov;
> +
> +		/* hwprov may technically be protected by ops lock but
> +		 * not for devices with a phydev, see phy_link_topo_add_phy()
> +		 */
> +		hwprov = rtnl_dereference(dev->hwprov);
> +		/* Disable timestamp if it is the one selected */
> +		if (hwprov && hwprov->phydev == phydev) {
> +			rcu_assign_pointer(dev->hwprov, NULL);
> +			kfree_rcu(hwprov, rcu_head);
> +		}
> +
> +		phydev->attached_dev->phydev = NULL;
> +		phydev->attached_dev = NULL;

[Severity: High]
This is a pre-existing issue, but does this catastrophically destroy
existing PHY attachments when called from the phy_attach_direct() error
path?

If phy_attach_direct() is called for a PHY that is already attached (e.g.,
due to a device tree misconfiguration), it correctly detects this with
"if (phydev->attached_dev)", but then jumps to the error label which calls
into this detachment path.

This unconditionally destroys the existing valid attachment (clearing
attached_dev->phydev and resetting PHY state) instead of gracefully
aborting the new attachment attempt. Additionally, balancing the old
attachment's references here permanently leaks the new module and device
references acquired earlier in phy_attach_direct().

> +		phy_link_topo_del_phy(dev, phydev);
> +	}
> +
> +	phydev->phy_link_change = NULL;

[ ... ]

> +	/* Assert the reset signal */
> +	phy_device_reset(phydev, 1);
> +
> +	/*
> +	 * The phydev might go away on the put_device() below, so avoid
> +	 * a use-after-free bug by reading the underlying bus first.
> +	 */
> +	bus = phydev->mdio.bus;
> +
> +	put_device(&phydev->mdio.dev);
> +	if (dev)
> +		ndev_owner = dev->dev.parent->driver->owner;
> +	if (ndev_owner != bus->owner)
> +		module_put(bus->owner);

[Severity: High]
This isn't a bug introduced by this patch, but can this dereference a
freed bus pointer?

If the MDIO bus has been unregistered and its primary references dropped,
the PHY device may hold the final reference to the bus via its parent
pointer. When phy_detach_internal() calls put_device(&phydev->mdio.dev),
it can drop the last reference to the bus, causing the struct mii_bus to
be freed.

The subsequent check "if (ndev_owner != bus->owner)" and
module_put(bus->owner) will then dereference the freed bus pointer.

> +}
> +
> +/**
> + * phy_detach - detach a PHY device from its network device

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4