Re: [PATCH net] net: phy: micrel: Fix potential shared PHC use-after-free

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 2 · 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] lan8814_ts_info() now calls mutex_lock(&shared->shared_lock),
  which can sleep, but the ethtool core always calls this callback…
- [Medium] After a member leaves, the shared PHC stays registered as a
  device-model child of the first PHY's mdio.dev.
- [Medium] lan8814_ptp_leave() uses shared->clock_stopping as a gate for
  a code region.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the same
  driver's LAN8841 PHC has exactly the use-after-free this commit…

--

net: phy: micrel: Fix potential shared PHC use-after-free

The patch moves the LAN8814/LAN8842 shared PHC pin array out of devm and
into package scope, and tracks which package members are bound. When a
member leaves, register access moves to a remaining PHY. The clock is
unregistered when the last member leaves, and shared_lock now also guards
ts_info and the GPIO interrupt path.

> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index d0b1ad21667b6..8c48d00b0ccf6 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c

[ ... ]

> @@ -3257,7 +3261,13 @@ static int lan8814_ts_info(struct mii_timestamper *mii_ts, struct kernel_ethtool
>  				SOF_TIMESTAMPING_RX_HARDWARE |
>  				SOF_TIMESTAMPING_RAW_HARDWARE;
>  
> +	mutex_lock(&shared->shared_lock);
> +	if (!shared->ptp_clock || shared->clock_stopping) {
> +		mutex_unlock(&shared->shared_lock);
> +		return -ENODEV;
> +	}
>  	info->phc_index = ptp_clock_index(shared->ptp_clock);
> +	mutex_unlock(&shared->shared_lock);

[Severity: High]
Can this mutex_lock() sleep inside an RCU read-side critical section? The
ethtool core calls the ts_info callback with rcu_read_lock() held:

net/ethtool/common.c:__ethtool_get_ts_info() {
	rcu_read_lock();
	hwprov = rcu_dereference(dev->hwprov);
	...
		if (phy_is_default_hwtstamp(phydev) &&
		    phy_has_tsinfo(phydev)) {
			err = phy_ts_info(phydev, info);
	...
	err = ethtool_get_ts_info_by_phc(dev, info, &hwprov->desc);
	rcu_read_unlock();
	return err;
}

phy_ts_info() calls phydev->mii_ts->ts_info() directly. For LAN8814 and
LAN8842 that callback is lan8814_ts_info().

shared_lock is also held across slow MDIO accesses by gettime64, adjfine,
adjtime and the GPIO interrupt thread. While ptp4l or phc2sys is running,
this path can block for real.

Any user can reach it with ethtool -T, ETHTOOL_MSG_TSINFO_GET, the
ETHTOOL_GET_TS_INFO ioctl, or SO_TIMESTAMPING with
SOF_TIMESTAMPING_BIND_PHC. With CONFIG_DEBUG_ATOMIC_SLEEP or PROVE_RCU,
won't each call print a "sleeping function called from invalid context"
splat? On non-preemptible RCU kernels, won't it hit "scheduling while
atomic"?

[ ... ]

> @@ -4636,12 +4649,90 @@ static int __lan8814_ptp_probe_once(struct phy_device *phydev, char *pin_name,
>  	return 0;
>  }
>  
> -static int lan8814_ptp_probe_once(struct phy_device *phydev)
> +static void lan8814_ptp_leave(void *data)
> +{
> +	struct phy_device *phydev = data;
> +	struct lan8814_shared_priv *shared = phy_package_get_priv(phydev);
> +	struct phy_device *next = NULL;
> +	unsigned int i;
> +
> +	phy_package_lock(phydev);
> +	mutex_lock(&shared->shared_lock);
> +	shared->members[phydev->mdio.addr] = NULL;
> +	for (i = 0; i < ARRAY_SIZE(shared->members); i++) {
> +		if (shared->members[i]) {
> +			next = shared->members[i];
> +			break;
> +		}
> +	}
> +	if (next) {
> +		shared->phydev = next;
> +		mutex_unlock(&shared->shared_lock);
> +		phy_package_unlock(phydev);
> +		return;
> +	}

[Severity: Medium]
This handoff only switches shared->phydev. The PHC device is still a child
of the PHY that registered it, because lan8814_ptp_register() does:

	shared->ptp_clock = ptp_clock_register(&shared->ptp_clock_info,
					       &phydev->mdio.dev);

and ptp_clock_register() sets ptp->dev.parent once, at registration time.

mdiobus_unregister() removes PHYs in ascending address order. For the
registering PHY, phy_device_remove()->device_del(&phydev->mdio.dev) first
runs lan8814_ptp_leave() and takes this handoff path. It then deletes the
device and its sysfs subtree, including the ptpN directory with its pins
and n_vclocks attributes, while /dev/ptpN is still live.

The last member then calls ptp_clock_unregister()->device_del() on a child
whose parent has already been deleted.

Is this parent-before-child removal order intended? After a sysfs unbind
of the first PHY, the PHC also stays parented under a PHY with no driver
bound, so the ptpN to device link userspace sees is wrong.

I don't see a crash from this. sysfs_remove_group() only pr_debug()s, and
the child still holds a reference on the parent kobject. But isn't the
PHC's device-model lifetime still tied to the first member, even though
the commit is meant to fix PHC lifetime across package members?

> +
> +	shared->clock_stopping = true;
> +	mutex_unlock(&shared->shared_lock);
> +	/* Unregister calls enable() to disable events, using the last PHY. */
> +	ptp_clock_unregister(shared->ptp_clock);
> +	mutex_lock(&shared->shared_lock);
> +	shared->ptp_clock = NULL;
> +	shared->phydev = NULL;
> +	kfree(shared->pin_config);
> +	shared->pin_config = NULL;
> +	shared->clock_stopping = false;
> +	mutex_unlock(&shared->shared_lock);
> +	phy_package_unlock(phydev);
> +}

[Severity: Medium]
Is shared->clock_stopping acting as an ad-hoc lock here? It is set before
the unlocked ptp_clock_unregister() call and cleared after it.
lan8814_ts_info() and lan8814_handle_gpio_interrupt() check it to avoid
shared->ptp_clock during that window. Lockdep can't see this gate.

The flag seems to exist because shared_lock can't be held across
ptp_clock_unregister(): ptp_disable_all_events() calls info->enable() ->
lan8814_ptp_extts(), which takes shared_lock.

One side effect: an EXTTS capture interrupt can arrive in this window,
before ptp_disable_all_events() clears PTP_COMMON_INT_ENA_GPIO_CAP_EN.
lan8814_handle_gpio_interrupt() then skips the capture:

	if (shared->ptp_clock && !shared->clock_stopping)
		ret = lan8814_gpio_process_cap(shared);
	else
		ret = -ENODEV;

PTP_GPIO_CAP_STS is never read, and lan8814_handle_interrupt() and
lan8842_handle_interrupt() treat the -ENODEV as not handled.
lan8814_ts_info() also returns -ENODEV during the window.

Could this teardown use a real lock or RCU instead of a flag?

[ ... ]

> @@ -7006,6 +7106,7 @@ static struct phy_driver ksphy_driver[] = {
>  	.flags		= PHY_POLL_CABLE_TEST,
>  	.driver_data	= &lan8814_type,
>  	.probe		= lan8842_probe,
> +	.remove		= lan8814_remove,
>  	.config_init	= lan8842_config_init,
>  	.config_intr	= lan8842_config_intr,
>  	.inband_caps	= lan8842_inband_caps,

[Severity: High]
This isn't a bug introduced by this patch, but the LAN8841 PHC in the same
driver seems to have the same use-after-free this commit describes, and it
is left unfixed.

lan8841_probe() allocates ptp_priv->pin_config with
devm_kcalloc(&phydev->mdio.dev, ...). ptp_priv->ptp_clock_info is embedded
in the devm-allocated kszphy_priv. The clock is registered with:

	ptp_priv->ptp_clock = ptp_clock_register(&ptp_priv->ptp_clock_info,
						 &phydev->mdio.dev);

The LAN8841 phy_driver entry has no .remove, and no devm action is added.
After this patch, the only ptp_clock_unregister() in micrel.c is the one
in lan8814_ptp_leave().

When a LAN8841 PHY is unbound (sysfs unbind, or MAC/MDIO bus removal),
devres frees priv and pin_config. /dev/ptpN stays registered with
ptp->info pointing into the freed memory.

Wouldn't a later PTP_CLOCK_GETCAPS, a clock_gettime() through
info->gettime64, or a pin sysfs access then use freed memory? A ptp4l that
still holds the fd open would hit this.

Should LAN8841 get the same treatment, either in this patch or in a
follow-up?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107-0d0c29e10a-net-micrel-lan8814-1%40mail.sdu.edu.cn