Re: [PATCH net v2 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 ENET interrupts (`fec_enet_interrupt`) are vulnerable to a use-after-free because they are devm-managed and outlive `free_netdev()`.
- [High] Teardown asymmetry allows tx_timeout_work to be rescheduled after cancellation.
- [High] Teardown asymmetry allows perout_timer to be re-armed after cancellation by a concurrent PTP ioctl.
--

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

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

This commit resolves a use-after-free issue by explicitly releasing the
PPS interrupt via devm_free_irq() in fec_ptp_stop(). This ensures the
interrupt handler is removed before the PHC and netdev are torn down.

> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 56801c2009d59..567ea2a461173 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
[ ... ]
> @@ -861,6 +865,12 @@ void fec_ptp_stop(struct platform_device *pdev)
>  
>  	cancel_delayed_work_sync(&fep->time_keep);
>  	hrtimer_cancel(&fep->perout_timer);

[Severity: High]
This is a pre-existing issue, but does this teardown asymmetry allow the
perout_timer to be re-armed after cancellation?

In fec_ptp_enable(), the ptp_clk_mutex is unlocked before hrtimer_start()
is called:

drivers/net/ethernet/freescale/fec_ptp.c:fec_ptp_enable() {
    ...
    hrtimer_start(&fep->perout_timer, timeout, HRTIMER_MODE_REL);
}

fec_ptp_stop() cancels the timer here, but doesn't hold any locks preventing
fec_ptp_enable() from running, nor does it immediately block new ioctls. Could
a concurrent PTP ioctl pass the clock-enabled check, get preempted, and then
call hrtimer_start() after the hrtimer_cancel() call above, causing the timer
to fire on freed memory?

> +
> +	if (fep->pps_irq >= 0) {
> +		devm_free_irq(&pdev->dev, fep->pps_irq, ndev);
> +		fep->pps_irq = -1;
> +	}
> +
>  	if (fep->ptp_clock)
>  		ptp_clock_unregister(fep->ptp_clock);
>  }

While reviewing this patch, I noticed a couple of other pre-existing teardown
issues in the FEC driver that might be vulnerable to similar races.

[Severity: High]
This isn't a bug introduced by this patch, but are the main ENET interrupts
vulnerable to a use-after-free during device removal?

The main interrupts are requested as devm-managed resources in fec_probe():

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

In fec_drv_remove(), the netdev is freed before these devm-managed
interrupts are released:

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

Since devm-managed resources are freed after the remove callback returns,
could an in-flight or spurious interrupt fire after free_netdev() and cause
fec_enet_interrupt() to dereference the freed ndev pointer?

[Severity: High]
This is also a pre-existing issue, but does the teardown sequence allow
tx_timeout_work to be rescheduled after it has been canceled?

In fec_drv_remove(), the work is canceled before the netdev watchdog is
stopped:

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

If the netdev watchdog fires in this window before unregister_netdev()
executes, it calls the ndo_tx_timeout handler which re-arms the work:

drivers/net/ethernet/freescale/fec_main.c:fec_timeout() {
    ...
    schedule_work(&fep->tx_timeout_work);
    ...
}

Could this cause the work item to be executed after the netdev has been
freed?

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