Re: [PATCH net 2/2] net: phy: xilinx-gmii2rgmii: Restore PHY driver on remove

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] xgmiitorgmii_remove()
  (drivers/net/phy/xilinx_gmii2rgmii.c:147-149) always writes the driver…
- [High] The fix relies on every reader of phydev->drv holding
  phydev->lock, and that is not true.

--

net: phy: xilinx-gmii2rgmii: Restore PHY driver on remove

This adds a remove callback to the GMII-to-RGMII converter. The callback
restores the attached PHY's original phydev->drv under phydev->lock, so
phydev->drv no longer points at the devres-freed conv_phy_drv copy after
the converter is unbound. It also drops the device reference taken by
of_phy_find_device().

> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
> index 61f71e977a574..b9aa5515577b5 100644
> --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> @@ -128,10 +128,29 @@ static int xgmiitorgmii_probe(struct mdio_device *mdiodev)

[ ... ]

> +static void xgmiitorgmii_remove(struct mdio_device *mdiodev)
> +{
> +	struct gmii2rgmii *priv = mdiodev_get_drvdata(mdiodev);
> +
> +	/*
> +	 * The attached PHY is a separate, still-bound device whose state
> +	 * machine keeps running and dispatches ->read_status / ->set_loopback
> +	 * under phydev->lock. Restore its original driver under that lock so
> +	 * the swap cannot race an in-flight dispatch; the restored driver is
> +	 * the PHY's own static phy_driver, not the devres-freed conv_phy_drv.
> +	 */
> +	mutex_lock(&priv->phy_dev->lock);
> +	priv->phy_dev->drv = priv->phy_drv;

[Severity: High]
Is it safe to write priv->phy_drv back here without first checking that
phydev->drv still points at &priv->conv_phy_drv?

The only thing the converter holds is the device reference from
of_phy_find_device(). There is no device link and no module reference on
the PHY driver. That means the PHY can be unbound or rebound while the
converter stays bound, and in that case phylib has already changed
phydev->drv:

drivers/net/phy/phy_device.c:phy_remove() {
    ...
	phydev->drv = NULL;

	return 0;
}

A rebind does the same thing: phy_probe() installs a new driver with
phydev->drv = phydrv.

If the converter is unbound after that, xgmiitorgmii_remove() writes back
the stale pointer that xgmiitorgmii_probe() saved. Three outcomes look
possible.

First, an unbound PHY gets a non-NULL drv again. That passes the
"if (!phydev->drv)" guards in phy.c and phy_device.c. The old driver's
callbacks then run against a phydev->priv that devres already freed. If
the PHY driver module was unloaded, they run in freed module text.

Second, if the PHY was rebound to another driver (for example Generic
PHY), the old driver's ops get installed over the new binding. They then
see the new driver's phydev->priv.

Third, if the converter is probed again, it passes the
!priv->phy_dev->drv check in xgmiitorgmii_probe() and memcpy()s from the
stale driver struct.

The comment above assumes the PHY is still bound and that the restored
driver is the PHY's own phy_driver. Nothing seems to enforce either
assumption.

Would it be better to restore only when phydev->drv == &priv->conv_phy_drv,
and to tie the converter's lifetime to the PHY binding?

> +	mutex_unlock(&priv->phy_dev->lock);

[Severity: High]
Does taking phydev->lock here actually prevent the use-after-free on
conv_phy_drv? The commit message says:

    Restore the attached PHY's original driver while
    holding phydev->lock so that the update cannot race with an active PHY
    callback.

The lock only excludes readers that also hold phydev->lock, and not every
reader of phydev->drv does. Once xgmiitorgmii_remove() returns,
devres_release_all() frees priv, and conv_phy_drv along with it.

The state machine drops the lock before its suspend step:

drivers/net/phy/phy.c:phy_state_machine() {
    ...
	mutex_lock(&phydev->lock);
	state_work = _phy_state_machine(phydev);
	mutex_unlock(&phydev->lock);

	_phy_state_machine_post_work(phydev, state_work);
}

phy_stop() does the same. _phy_state_machine_post_work() then calls
phy_suspend(), which copies phydev->drv into a local and dereferences it
with no lock held:

drivers/net/phy/phy_device.c:phy_suspend() {
    ...
	const struct phy_driver *phydrv = phydev->drv;
    ...
	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
		return -EBUSY;

	if (!phydrv->suspend)
		return 0;

	ret = phydrv->suspend(phydev);
    ...
}

phy_ethtool_get_sset_count() also dereferences the driver before it takes
the mutex. It is reachable from the ethtool ioctl through
ethtool_get_phy_stats_phydev():

drivers/net/phy/phy.c:phy_ethtool_get_sset_count() {
    ...
	if (phydev->drv->get_sset_count &&
	    phydev->drv->get_strings &&
	    phydev->drv->get_stats) {
		mutex_lock(&phydev->lock);
    ...
}

Could this sequence happen?

CPU0                                  CPU1
phy_suspend()
  phydrv = &priv->conv_phy_drv
                                      xgmiitorgmii_remove()
                                        mutex_lock(&phydev->lock)
                                        phydev->drv = priv->phy_drv
                                        mutex_unlock(&phydev->lock)
                                      devres_release_all()
                                        kfree(priv)
  phydrv->flags / phydrv->suspend

If so, CPU0 would read freed memory and might call through it. That is the
same kind of use-after-free this patch is meant to fix.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001074718.3944521-1-vineeth.karumanchi%40amd.com