Re: [PATCH net v3 2/2] net: fec: free the PPS interrupt before tearing down the PHC and netdev

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The main ethernet interrupts are managed via devres but `netdev` is freed manually, causing a use-after-free if an interrupt fires after driver removal.
- [High] Unregistering the PTP clock after canceling timers and freeing IRQs allows concurrent ioctls to re-arm them, leading to Use-After-Free.
- [High] Race condition causing tx_timeout_work to be queued after it is cancelled, leading to a use-after-free when the work executes.
--

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

net: fec: free the PPS interrupt before tearing down the PHC and netdev

The devm-managed PPS interrupt remains registered until after the remove
callback returns, outliving both the PHC and the netdev. This patch uses
request_irq() and free_irq() to explicitly manage the interrupt lifetime.

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
[ ... ]
> @@ -861,6 +863,12 @@ void fec_ptp_stop(struct platform_device *pdev)
>  
>  	cancel_delayed_work_sync(&fep->time_keep);
>  	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);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but does unregistering the PTP clock after
canceling timers and freeing IRQs allow concurrent ioctls to re-arm them?

In fec_ptp_stop(), the driver cancels the perout_timer hrtimer before calling
ptp_clock_unregister(). Because the PTP char device is still registered and
active, a concurrent userspace thread can issue a PTP clock request ioctl
which calls fec_ptp_enable() and re-arms the timer via hrtimer_start().

fec_ptp_stop() finishes, and fec_drv_remove() frees the netdev. When the
re-armed timer fires, its callback fec_ptp_pps_perout_handler() accesses the
freed fep structure, leading to a use-after-free.

Should ptp_clock_unregister() be called before tearing down the timers and
interrupts to prevent them from being re-armed?


[Severity: High]
This is a pre-existing issue, but shares the same root cause as the PPS
interrupt bug this patch is fixing. Does the driver also have a use-after-free
risk with the main ethernet interrupts?

In fec_probe(), the main ethernet interrupts are requested using devres:

drivers/net/ethernet/freescale/fec_main.c:fec_probe() {
    ...
    ret = devm_request_irq(&pdev->dev, irq, fec_enet_interrupt,
                           0, pdev->name, ndev);
    ...
}

However, in fec_drv_remove(), the netdev is freed manually:

drivers/net/ethernet/freescale/fec_main.c:fec_drv_remove() {
    ...
    free_netdev(ndev);
    ...
}

Because devm_request_irq() ties the interrupt lifetime to the device rather
than the netdev lifecycle, the interrupts are not freed until after
fec_drv_remove() returns. If an interrupt fires in this window,
fec_enet_interrupt() will execute and dereference the freed ndev pointer.

Should the main ethernet interrupts also be converted to request_irq() and
freed explicitly in fec_drv_remove() before free_netdev() is called?


[Severity: High]
This is a pre-existing issue, but is there a race condition causing
tx_timeout_work to be queued after it is cancelled?

In fec_drv_remove(), cancel_work_sync() is called before unregister_netdev():

drivers/net/ethernet/freescale/fec_main.c:fec_drv_remove() {
    ...
    cancel_work_sync(&fep->tx_timeout_work);
    unregister_netdev(ndev);
    ...
}

While unregister_netdev() is executing, the netdev is still active, allowing
the networking core watchdog to detect a transmit timeout and invoke
ndo_tx_timeout (fec_timeout()). This asynchronously re-queues tx_timeout_work.

The driver continues teardown and frees the netdev. When the workqueue
executes fec_enet_timeout_work(), it accesses the freed fep pointer, causing a
use-after-free.

Should unregister_netdev() be called before cancel_work_sync() to ensure no
new work is queued?

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