Re: [PATCH net-next v19 01/13] phylink: Add phylink_mac_interrupt

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] phylink: UAF and TOCTOU in phylink_mac_interrupt due to lockless phydev access
--

commit 667c3fb1a09bf0994b40559aee9027d3f9775442
Author: Birger Koblitz <mail@birger-koblitz.de>

phylink: Add phylink_mac_interrupt

Adds the phylink_mac_interrupt() function to phylink, passing link-change
interrupts received by the MAC to phylib.

> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index a7d086cdc9b25..a3ca7993eb7e0 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
[ ... ]
> @@ -1612,6 +1612,26 @@ static bool phylink_link_is_up(struct phylink *pl)
>  	return pl->netdev ? netif_carrier_ok(pl->netdev) : pl->old_link_state;
>  }
>  
> +/**
> + * phylink_mac_interrupt() - wrapper for phy_mac_interrupt()
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + *
> + * Passes a link-change interrupt received by the MAC to phylib.
> + * This function may be called in interrupt context, and therefore cannot take
> + * a mutex. However, it is safe to assume that the reference to phy does not
> + * disappear during the call, as only an SFP module may be unplugged to make a
> + * PHY disappear. SFP modules however do not support interrupts.
> + */
> +void phylink_mac_interrupt(struct phylink *pl)
> +{
> +	struct phy_device *phy;
> +
> +	phy = pl->phydev;
> +	if (phy)
> +		phy_mac_interrupt(phy);

[Severity: High]
The function comment notes that SFP modules do not support interrupts. However,
when an SFP module is physically unplugged, the electrical link drops. Could
this link drop trigger the MAC interrupt being handled here?

If so, can this race with the concurrent SFP unplug worker and cause a
use-after-free?

The SFP unplug worker concurrently clears the pointer without RCU
synchronization:

drivers/net/phy/phylink.c:phylink_disconnect_phy() {
    ...
    pl->phydev = NULL;
    ...
}

And then immediately frees the PHY device:

drivers/net/phy/sfp.c:sfp_sm_phy_detach() {
    ...
    phy_device_free(sfp->mod_phy);
    ...
}

Because phylink_mac_interrupt() reads pl->phydev locklessly and without an
RCU read-side critical section, is it possible for the interrupt to fetch the
pointer right before the worker frees it?

Additionally, does the lack of READ_ONCE() here allow the compiler to reload
pl->phydev after the if (phy) check, potentially causing a time-of-check to
time-of-use NULL pointer dereference?

> +}
> +EXPORT_SYMBOL_GPL(phylink_mac_interrupt);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-ax88179a-v19-0-7475173cfb56@birger-koblitz.de?part=1