Re: [PATCH net v3 1/2] net: fec: stop the PTP clock after the netdev is unregistered

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The network device and its private data are freed before devm-managed interrupts are unregistered, leading to a potential Use-After-Free if an interrupt fires during teardown.
- [High] The tx_timeout_work is cancelled before unregister_netdev, allowing the watchdog to reschedule it before the device is fully closed, leading to a Use-After-Free.
- [High] PTP clock teardown race allows userspace to re-enable hardware interrupts and timers after they are disabled.
- [High] The PTP perout timer can fire and access hardware registers while the PTP clock is disabled.
- [Medium] The global variables `mii_cnt` and `fec0_mii_bus` are mutated without locking during probe and remove.
--

commit d3e2d320b9d2d80773ed19690bcf91e7ed19f611
Author: Shengzhuo Wei <me@cherr.cc>

net: fec: stop the PTP clock after the netdev is unregistered

This commit moves the PTP clock stop function in the driver's remove
path to execute after the netdev is unregistered. This prevents a race
condition where concurrent ethtool callbacks might access a freed PTP
clock.

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 794ec427b0ee6..a29c8ddbbf34d 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5500,9 +5500,9 @@ fec_drv_remove(struct platform_device *pdev)
>  			ERR_PTR(ret));
>  
>  	cancel_work_sync(&fep->tx_timeout_work);

[Severity: High]
This is a pre-existing issue, but by cancelling tx_timeout_work before
unregister_netdev(ndev), is there a window where the network watchdog can
still fire and reschedule the work?

unregister_netdev() stops the network device watchdog via dev_close(). In
the window between cancel_work_sync() and dev_close(), a transmit timeout
can still occur. If this happens, fec_timeout() can run and reschedule
the work:

    schedule_work(&fep->tx_timeout_work);

Could this lead to a use-after-free on fep and ndev if the scheduled work
executes after the device teardown is complete?

> +	unregister_netdev(ndev);
>  	if (fep->bufdesc_ex)
>  		fec_ptp_stop(pdev);

[Severity: High]
This is a pre-existing issue, but could the PTP perout timer fire and
access hardware registers while the PTP clock is disabled?

When unregister_netdev() is called, it brings down the interface, which
calls fec_enet_close(). This in turn disables clk_ptp.

Because the perout_timer is not cancelled until later in fec_ptp_stop(),
the timer might fire after the clock is disabled. If this happens,
fec_ptp_pps_perout() will access PTP registers:

    temp_val = readl(fep->hwp + FEC_ATIME_CTRL);
    temp_val |= FEC_T_CTRL_PINPER;
    writel(temp_val, fep->hwp + FEC_ATIME_CTRL);

Without checking if the clock is running, wouldn't accessing these registers
trigger an external synchronous abort (kernel panic) on ARM?

[Severity: High]
This is also a pre-existing issue, but does fec_ptp_stop() leave a race 
window that allows userspace to re-enable timers and interrupts?

Inside fec_ptp_stop(), timers and features are disabled before the PTP
clock is unregistered:

    hrtimer_cancel(&fep->perout_timer);

    if (fep->pps_irq >= 0) {
        free_irq(fep->pps_irq, ndev);
        fep->pps_irq = -1;
    }

    if (fep->ptp_clock)
        ptp_clock_unregister(fep->ptp_clock);

During this intermediate window, could concurrent userspace ioctls (such
as PTP_CLK_REQ_PPS) execute and re-enable the PPS hardware interrupt or
restart the hrtimer?

> -	unregister_netdev(ndev);
>  	fec_enet_mii_remove(fep);

[Severity: Medium]
This is a pre-existing issue, but is it safe to mutate the shared mii_cnt 
variable without locking during probe and remove?

fec_enet_mii_init() increments mii_cnt, and fec_enet_mii_remove() decrements
it. The driver uses these static global variables to share a single MDIO
bus between multiple FEC MAC instances.

If instances probe or remove asynchronously, or via hotplug, could concurrent
modifications corrupt the reference counter? This might lead to a memory leak
or premature unregistration of an actively used MDIO bus.

>  	if (fep->reg_phy)
>  		regulator_disable(fep->reg_phy);

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence
leave the primary interrupt vulnerable to a use-after-free?

In fec_probe(), the primary interrupt is requested using a devm-managed
function:

    ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt, 0,
                           pdev->name, ndev);

However, later in fec_drv_remove(), free_netdev(ndev) is called
synchronously. Since devres cleanup (which frees the IRQ) only happens after 
fec_drv_remove() completes, the ndev pointer is freed while the IRQ is still
active. If a spurious or delayed interrupt fires in this window, it would
result in a use-after-free on ndev.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-fec-ptp-pps-event-uaf-v3-0-bead12c29a16@cherr.cc?part=1