Re: [PATCH 2/2] net: fec: Handle PTP initialization errors in probe

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

Pre-existing issues:
- [High] The patch fixes an unconditional `fec_ptp_stop()` in the probe error path but misses the identical bug in `fec_drv_remove()`, leading to a NULL pointer dereference.
- [High] Use-after-free in devm-managed interrupt handlers during driver removal due to premature `free_netdev()`.
- [High] PTP device is exposed to userspace before probe completes, leading to potential bus faults on probe error paths.
- [High] `INIT_WORK(&fep->tx_timeout_work)` is called after `register_netdev()`, creating a race window where an uninitialized work queue can be scheduled.
--

--- Patch [2]: [PATCH 2/2] net: fec: Handle PTP initialization errors in probe ---
commit 41e2a49ed28a7c17df87f2ff80ee17b2b80b3801
Author: bui duc phuc <[email protected]>

net: fec: Handle PTP initialization errors in probe
    
Check the return value of fec_ptp_init() and abort the probe if PTP
initialization fails. Only call fec_ptp_stop() when PTP support was
initialized to avoid stopping an uninitialized PTP instance during error
handling.

> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index ced4dbf8cd90f..b6c22d6c1e69a 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -5384,8 +5384,11 @@ fec_probe(struct platform_device *pdev)
>  		goto failed_reset;
>  
>  	irq_cnt = fec_enet_get_irq_cnt(pdev);
> -	if (fep->bufdesc_ex)
> -		fec_ptp_init(pdev, irq_cnt);
> +	if (fep->bufdesc_ex) {
> +		ret = fec_ptp_init(pdev, irq_cnt);
> +		if (ret)
> +			goto failed_reset;
> +	}

[Severity: High]
This is a pre-existing issue, but does fec_ptp_init() expose the PTP device
to userspace before the probe completes?

drivers/net/ethernet/freescale/fec_ptp.c:fec_ptp_init() {
    ...
    fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
    ...
}

This makes the character device accessible to userspace before network device
registration completes. If register_netdev() fails later in fec_probe(), the
error path shuts down hardware clocks.

If userspace accesses the PTP device concurrently during this window (e.g. by
issuing an ioctl), could the PTP callbacks attempt to access disabled
hardware registers?

>  
>  	ret = fec_enet_init(ndev);
>  	if (ret)
[ ... ]
> @@ -5457,7 +5460,8 @@ fec_probe(struct platform_device *pdev)
>  failed_irq:
>  	fec_enet_deinit(ndev);
>  failed_init:
> -	fec_ptp_stop(pdev);
> +	if (fep->bufdesc_ex)
> +		fec_ptp_stop(pdev);

[Severity: High]
This isn't a bug introduced by this patch, but since you are fixing the
unconditional call to fec_ptp_stop() here in the probe error path, should we
also fix the exact same issue in fec_drv_remove()?

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

When the driver is unbound from a device without PTP support,
fec_drv_remove() unconditionally calls fec_ptp_stop(). Since PTP was never
initialized, hrtimer_cancel() inside fec_ptp_stop() will read from a
zero-initialized timer, dereferencing a null base pointer.

[Severity: High]
This is a pre-existing issue, but does fec_drv_remove() have a use-after-free
risk with devm-managed interrupts?

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

The interrupt is requested with devm_request_irq() in fec_probe(), meaning
its lifetime extends beyond fec_drv_remove(). However, fec_drv_remove()
explicitly calls free_netdev(), freeing the ndev and fep structures.

If a spurious or shared interrupt fires after free_netdev() returns but
before devres executes free_irq(), could the interrupt handler dereference
the freed memory?

[Severity: High]
This isn't a bug introduced by this patch, but does the initialization order
in fec_probe() create a race window for tx_timeout_work?

drivers/net/ethernet/freescale/fec_main.c:fec_probe() {
    ...
    ret = register_netdev(ndev);
    if (ret)
        goto failed_register;
    ...
    INIT_WORK(&fep->tx_timeout_work, fec_enet_timeout_work);
    ...
}

Calling register_netdev() makes the network device visible to the system, so
userspace can configure the interface and start transmitting traffic. 

If a hardware stall causes the ndo_tx_timeout callback to be invoked before
INIT_WORK() executes, could schedule_work(&fep->tx_timeout_work) operate on
an uninitialized work_struct?

>  failed_reset:
>  	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2