[PATCH net 0/2] net: fec: fix PTP clock teardown races on device removal

[PATCH net 0/2] net: fec: fix PTP clock teardown races on device removal

[Shengzhuo Wei]

fec_drv_remove() calls fec_ptp_stop() before the netdev is unregistered
and freed, but PTP teardown leaves dangling references behind.  The PHC
pointer remains set after ptp_clock_unregister(), so an ethtool -T on the
still-registered netdev can pass it to ptp_clock_index() after the clock
structure has been freed.

An in-flight PPS handler can race ptp_clock_unregister().  The devm-managed
handler also remains registered past free_netdev() and can dereference the
freed netdev before device-managed resources are released.

This series closes both holes at their respective introduction points.
Look up the PHC index by the parent device and clear fep->ptp_clock after
unregistering the PHC, then explicitly free the dedicated PPS interrupt
before the PHC and netdev teardown.

Found by source inspection while reviewing PTP teardown paths.  Verified by
compiling the driver with W=1.  No hardware was available to reproduce the
races.

---
Shengzhuo Wei (2):
      net: fec: don't leave a stale PTP clock pointer after unregister
      net: fec: free the PPS interrupt before tearing down the PHC and netdev

 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c |  3 +--
 drivers/net/ethernet/freescale/fec_ptp.c  | 15 +++++++++++++--
 3 files changed, 15 insertions(+), 4 deletions(-)
---
base-commit: 548e7bcd0c5460ddcbca9600cea603ebeebf4da7
change-id: 20260901-fec-ptp-pps-event-uaf-dcc71b5e1db0

Best regards,
--  
Shengzhuo Wei <me@cherr.cc>

[PATCH net 1/2] net: fec: don't leave a stale PTP clock pointer after unregister

[Shengzhuo Wei]

fec_drv_remove() calls fec_ptp_stop() before unregister_netdev(), and
fec_ptp_stop() leaves fep->ptp_clock set after ptp_clock_unregister().
An ethtool -T issued while the netdev is still registered then reaches
fec_enet_get_ts_info(), which passes the stale pointer to
ptp_clock_index() after the clock structure has been freed.

Query the PHC index through ptp_clock_index_by_dev() instead.  The lookup
holds a reference to the matching PTP class device while reading its index,
so concurrent unregister cannot free it underneath the lookup.  It returns
-1 once no clock is registered.  Clear fep->ptp_clock after unregistering
it as well, so the driver state no longer retains the invalid pointer.

Fixes: 32cba57ba74b ("net: fec: introduce fec_ptp_stop and use in probe fail path")
Cc: stable@vger.kernel.org
Assisted-by: GLM:5.3
Signed-off-by: Shengzhuo Wei <me@cherr.cc>
---
 drivers/net/ethernet/freescale/fec_main.c | 3 +--
 drivers/net/ethernet/freescale/fec_ptp.c  | 4 +++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 794ec427b0ee..0606559d495c 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3337,8 +3337,7 @@ static int fec_enet_get_ts_info(struct net_device *ndev,
 					SOF_TIMESTAMPING_TX_HARDWARE |
 					SOF_TIMESTAMPING_RX_HARDWARE |
 					SOF_TIMESTAMPING_RAW_HARDWARE;
-		if (fep->ptp_clock)
-			info->phc_index = ptp_clock_index(fep->ptp_clock);
+		info->phc_index = ptp_clock_index_by_dev(&fep->pdev->dev);
 
 		info->tx_types = (1 << HWTSTAMP_TX_OFF) |
 				 (1 << HWTSTAMP_TX_ON);
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 56801c2009d5..0036549974fd 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -861,6 +861,8 @@ void fec_ptp_stop(struct platform_device *pdev)
 
 	cancel_delayed_work_sync(&fep->time_keep);
 	hrtimer_cancel(&fep->perout_timer);
-	if (fep->ptp_clock)
+	if (fep->ptp_clock) {
 		ptp_clock_unregister(fep->ptp_clock);
+		fep->ptp_clock = NULL;
+	}
 }

-- 
2.47.3

RE: [PATCH net 1/2] net: fec: don't leave a stale PTP clock pointer after unregister

[Wei Fang]

> fec_drv_remove() calls fec_ptp_stop() before unregister_netdev(), and
> fec_ptp_stop() leaves fep->ptp_clock set after ptp_clock_unregister().
> An ethtool -T issued while the netdev is still registered then reaches
> fec_enet_get_ts_info(), which passes the stale pointer to
> ptp_clock_index() after the clock structure has been freed.
> 
> Query the PHC index through ptp_clock_index_by_dev() instead.  The lookup
> holds a reference to the matching PTP class device while reading its index,
> so concurrent unregister cannot free it underneath the lookup.  It returns
> -1 once no clock is registered.  Clear fep->ptp_clock after unregistering
> it as well, so the driver state no longer retains the invalid pointer.

Why not move fec_ptp_stop() after unregister_netdev()?

Re: [PATCH net 1/2] net: fec: don't leave a stale PTP clock pointer after unregister

[Shengzhuo Wei]

On 2026-09-04 03:15, Wei Fang wrote:
> Why not move fec_ptp_stop() after unregister_netdev()?

Good point. I was focused on making the PHC index lookup safe
independently of the teardown ordering and followed the approach used by
similar fixes, so I overlooked the simpler solution here.

Moving fec_ptp_stop() after unregister_netdev() is cleaner and restores
the proper lifetime ordering. I will rework patch 1 accordingly and drop
the ptp_clock_index_by_dev() change in v2.

Thanks,
Shengzhuo

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

[Shengzhuo Wei]

The dedicated PPS interrupt is devm-managed, so its handler remains
registered until device-managed resources are released after the remove
callback returns.  It therefore outlives both fec_ptp_stop() and
free_netdev().

A handler already in flight can observe pps_enable before fec_ptp_stop()
clears it and call ptp_clock_event() concurrently with
ptp_clock_unregister().  A handler invoked after free_netdev() but before
device-managed cleanup dereferences the freed netdev.

Record the IRQ after a successful request and release it explicitly in
fec_ptp_stop(), before unregistering the PHC.  devm_free_irq() removes the
handler and waits for any running instance, so none can still execute when
PHC teardown begins.  The IRQ is requested without IRQF_SHARED, so its
release does not affect another handler.

Fixes: 4ad1ceec05e4 ("net: fec: Let fec_ptp have its own interrupt routine")
Cc: stable@vger.kernel.org
Assisted-by: GLM:5.3
Signed-off-by: Shengzhuo Wei <me@cherr.cc>
---
 drivers/net/ethernet/freescale/fec.h     |  1 +
 drivers/net/ethernet/freescale/fec_ptp.c | 11 ++++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 7176803146f3..960b9f01c531 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -670,6 +670,7 @@ struct fec_enet_private {
 
 	/* pps  */
 	int pps_channel;
+	int pps_irq;
 	unsigned int reload_period;
 	int pps_enable;
 	unsigned int next_counter;
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 0036549974fd..5b1d58c85fcd 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -787,9 +787,12 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
 	if (irq >= 0) {
 		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
 				       0, pdev->name, ndev);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
 				 ret);
+		} else {
+			fep->pps_irq = irq;
+		}
 	}
 
 	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
@@ -861,6 +864,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) {
+		devm_free_irq(&pdev->dev, fep->pps_irq, ndev);
+		fep->pps_irq = 0;
+	}
+
 	if (fep->ptp_clock) {
 		ptp_clock_unregister(fep->ptp_clock);
 		fep->ptp_clock = NULL;

-- 
2.47.3

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Cancelling timers before unregistering the PTP clock allows concurrent PTP API calls to re-arm the timer, leading to a use-after-free.
- [High] The main Ethernet interrupts (`fec_enet_interrupt`) are subject to the exact same Use-After-Free vulnerability that this patch fixes for the PPS interrupt.
- [High] The hardirq handler fec_pps_interrupt accesses and modifies shared state without holding fep->tmreg_lock, leading to data races.
- [High] System crash (synchronous external abort) in `fec_ptp_stop()` when `pm_runtime_get_sync()` fails.
--

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

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

This patch explicitly frees the devm-managed PPS interrupt during
fec_ptp_stop() to prevent the interrupt handler from outliving the
network device. This resolves a use-after-free where an in-flight
handler could execute after free_netdev() is called.

> A handler already in flight can observe pps_enable before fec_ptp_stop()
> clears it and call ptp_clock_event() concurrently with
> ptp_clock_unregister().  A handler invoked after free_netdev() but before
> device-managed cleanup dereferences the freed netdev.

[Severity: High]
This isn't a bug introduced by this patch, but are the main Ethernet
interrupts (fec_enet_interrupt) subject to the exact same use-after-free
vulnerability that this patch fixes for the PPS interrupt?

In fec_main.c:fec_probe(), the main interrupts are requested via
devm_request_irq():

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

However, in fec_drv_remove(), free_netdev(ndev) is called while these
interrupts are still registered and active, which means they could fire
and access freed memory before devres cleanup runs later.

[ ... ]
> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
> index 0036549974fd3..5b1d58c85fcd2 100644
> --- a/drivers/net/ethernet/freescale/fec_ptp.c
> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
> @@ -787,9 +787,12 @@ void fec_ptp_init(struct platform_device *pdev, int irq_idx)
>  	if (irq >= 0) {
>  		ret = devm_request_irq(&pdev->dev, irq, fec_pps_interrupt,
>  				       0, pdev->name, ndev);

[Severity: High]
This is a pre-existing issue, but does fec_pps_interrupt() access and
modify shared state without holding fep->tmreg_lock, leading to data
races?

When the interrupt fires, fec_pps_interrupt() updates fep->next_counter
locklessly in hardirq context:

    fep->next_counter = (fep->next_counter + fep->reload_period) & fep->cc.mask;
    writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));

This can race with fec_ptp_enable_pps() and fec_ptp_pps_perout_handler(),
which correctly use spin_lock_irqsave(&fep->tmreg_lock, flags) to serialize
updates to the same state. Can this cause interleaved reads and writes that
corrupt the timer state and hardware registers on SMP systems?

> -		if (ret < 0)
> +		if (ret < 0) {
>  			dev_warn(&pdev->dev, "request for pps irq failed(%d)\n",
>  				 ret);
> +		} else {
> +			fep->pps_irq = irq;
> +		}
>  	}
>  
>  	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
> @@ -861,6 +864,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 cancelling the timer before
unregistering the PTP clock allow concurrent PTP API calls to re-arm
the timer and cause a use-after-free?

Because ptp_clock remains registered, userspace can concurrently invoke
PTP ioctls that call fec_ptp_enable(), which re-arms perout_timer. If
this race occurs and the timer is re-armed, it can fire after
fec_drv_remove() completes and free_netdev() is called, dereferencing
the freed ndev and fep.

Should ptp_clock_unregister() happen at the beginning of the teardown
sequence?

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

[Severity: High]
This isn't a bug introduced by this patch, but does unconditionally
calling fec_ptp_stop() during device removal lead to a synchronous
external abort if runtime PM fails?

In fec_main.c:fec_drv_remove(), pm_runtime_get_sync() is called, but if
it fails, execution continues and the device's clocks remain gated:

    ret = pm_runtime_get_sync(&pdev->dev);
    if (ret < 0)
        dev_err(&pdev->dev, ...);

    if (fep->bufdesc_ex)
        fec_ptp_stop(pdev);

If fep->pps_enable was left enabled, fec_ptp_stop() will attempt to
access raw hardware registers (via fec_ptp_enable_pps()) while the clocks
are gated, which triggers a kernel panic on i.MX platforms.

>  		fep->ptp_clock = NULL;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-fec-ptp-pps-event-uaf-v1-0-9af446be4a11@cherr.cc?part=2