Re: [PATCH net v2 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 4 potential issue(s) to consider:

New issues:
- [High] Moving fec_ptp_stop() after unregister_netdev() introduces a race condition that can cause a hard CPU lockup in the PPS interrupt handler.

Pre-existing issues:
- [High] Calling cancel_work_sync(&fep->tx_timeout_work) before unregister_netdev() creates a race condition that allows the TX timeout work to be rescheduled, leading to a Use-After-Free.
- [High] Active devm-managed IRQ handlers are exposed to Use-After-Free because `ndev` is explicitly freed before the devres device teardown phase completes.
- [High] Hardware accesses in PTP operations are performed without runtime PM protection and without checking the PTP clock state, leading to a potential Synchronous External Abort.
--

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

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

This commit addresses a race condition where a freed PTP clock could be
accessed during ethtool operations by moving fec_ptp_stop() after
unregister_netdev().

> 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 does calling cancel_work_sync() before
unregister_netdev() leave a window for the work to be rescheduled?

While unregister_netdev() is running, the network interface and its transmit
watchdog timer (dev_watchdog) are still active. If the watchdog timer fires
during this window, could fec_timeout() re-queue the work via
schedule_work(&fep->tx_timeout_work)?

Since unregister_netdev() shuts down the interface but does not cancel newly
scheduled work, the delayed work could run after free_netdev() destroys
ndev, leading to a Use-After-Free.

[Severity: High]
This isn't a bug introduced by this patch, but does the error handling for
pm_runtime_get_sync() safely protect the hardware accesses?

If pm_runtime_get_sync() fails, the device remains suspended, but execution
continues into fec_ptp_stop(), which invokes fec_ptp_enable_pps() and
writes to FEC_TCSR.

Similarly, PTP ioctls or deferred timers might invoke functions like
fec_ptp_adjfine(), which unconditionally access hardware registers:

fec_ptp.c:fec_ptp_adjfine() {
    ...
    tmp = readl(fep->hwp + FEC_ATIME_INC) & FEC_T_INC_MASK;
    ...
}

Could accessing these registers while the device bus clocks are disabled
cause a Synchronous External Abort (bus fault)?

> +	unregister_netdev(ndev);

[Severity: High]
Does moving unregister_netdev() before fec_ptp_stop() introduce a race
condition that can cause a hard CPU lockup in the PPS interrupt handler?

Calling unregister_netdev() brings the device down and invokes
fec_enet_close(), which disables the PTP clock via
fec_enet_clk_enable(ndev, false).

During the window before fec_ptp_stop() is called, the hardware PPS
interrupt is still registered and unmasked. If the PPS interrupt fires
in this window, the CPU enters fec_pps_interrupt() and tries to clear
the interrupt flag by writing to FEC_TCSR in a do-while loop:

fec_ptp.c:fec_pps_interrupt() {
    ...
    do {
        writel(val, fep->hwp + FEC_TCSR(channel));
    } while (readl(fep->hwp + FEC_TCSR(channel)) & FEC_T_TF_MASK);
    ...
}

Since the functional timer clock (clk_ptp) is disabled, the timer IP block
might not process the clear operation. Will the TF_MASK bit never clear,
causing the interrupt handler to spin indefinitely?

>  	if (fep->bufdesc_ex)
>  		fec_ptp_stop(pdev);
> -	unregister_netdev(ndev);
>  	fec_enet_mii_remove(fep);
>  	if (fep->reg_phy)
>  		regulator_disable(fep->reg_phy);

[Severity: High]
This is a pre-existing issue, but does explicitly freeing the netdev later
in this function expose devm-managed IRQ handlers to a Use-After-Free?

The driver registers network interrupts using devm_request_irq() in
fec_probe(), passing ndev as the dev_id token. These devm-managed
resources are only released by devres after fec_drv_remove() returns.

However, fec_drv_remove() manually calls free_netdev(ndev) at the end
of the function:

fec_main.c:fec_drv_remove() {
    ...
    fec_enet_deinit(ndev);
    free_netdev(ndev);
}

If a spurious or shared interrupt fires between the free_netdev() execution
and the devres teardown phase, could the active IRQ handler be invoked with
a freed ndev pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-fec-ptp-pps-event-uaf-v2-0-2a1d57121ade@cherr.cc?part=1