[PATCH net] net: enc28j60: fix use-after-free on remove

[PATCH net] net: enc28j60: fix use-after-free on remove

[Shengzhuo Wei]

enc28j60_remove() frees the net_device without cancelling the
tx_work, setrx_work, and restart_work items the ndo callbacks queue,
so a work racing with remove can run after free_netdev(). A queued
tx skb is also left waiting for its transmit-complete interrupt,
which never arrives, and leaks.

Unregister the netdev, free the IRQ, and cancel the works before
freeing the net_device, and release the leftover tx skb.

Fixes: 3ec9c11da033 ("add driver for enc28j60 ethernet chip")
Cc: stable@vger.kernel.org
Assisted-by: GLM:5.3
Signed-off-by: Shengzhuo Wei <me@cherr.cc>
---
Found by source inspection. Compile-tested with
CONFIG_SPI_ENC28J60=y and W=1; no ENC28J60 hardware was available,
so no runtime reproducer or hardware test was performed.
---
 drivers/net/ethernet/microchip/enc28j60.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/microchip/enc28j60.c b/drivers/net/ethernet/microchip/enc28j60.c
index d6c9491537e4..d16564060fbc 100644
--- a/drivers/net/ethernet/microchip/enc28j60.c
+++ b/drivers/net/ethernet/microchip/enc28j60.c
@@ -1602,6 +1602,11 @@ static void enc28j60_remove(struct spi_device *spi)
 
 	unregister_netdev(priv->netdev);
 	free_irq(spi->irq, priv);
+	cancel_work_sync(&priv->tx_work);
+	cancel_work_sync(&priv->setrx_work);
+	cancel_work_sync(&priv->restart_work);
+	if (priv->tx_skb)
+		dev_kfree_skb(priv->tx_skb);
 	free_netdev(priv->netdev);
 }
 

---
base-commit: 45c13f3f9e3bb15fd89ff2864c6f627a3b4b4229
change-id: 20260827-enc28j60-cancel-works-62a0e02806af

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

Re: [net] net: enc28j60: fix use-after-free on remove

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The deferred work items are never quiesced before the
  controller is powered down, and the patch adds the cancels only at the…

--

net: enc28j60: fix use-after-free on remove

enc28j60_remove() now unregisters the netdev, frees the IRQ, cancels the
tx_work, setrx_work and restart_work items, and frees a leftover tx skb
before calling free_netdev().

> diff --git a/drivers/net/ethernet/microchip/enc28j60.c b/drivers/net/ethernet/microchip/enc28j60.c
> index d6c9491537e4..d16564060fbc 100644
> --- a/drivers/net/ethernet/microchip/enc28j60.c
> +++ b/drivers/net/ethernet/microchip/enc28j60.c
> @@ -1602,6 +1602,11 @@ static void enc28j60_remove(struct spi_device *spi)
>  
>  	unregister_netdev(priv->netdev);
>  	free_irq(spi->irq, priv);
> +	cancel_work_sync(&priv->tx_work);
> +	cancel_work_sync(&priv->setrx_work);
> +	cancel_work_sync(&priv->restart_work);

[Severity: Medium]

Is the placement of these cancels after unregister_netdev() and
free_irq() intentional?

unregister_netdev() runs dev_close(), and enc28j60_net_close() puts the
part to sleep without quiescing any of the works:

drivers/net/ethernet/microchip/enc28j60.c:enc28j60_net_close() {
	enc28j60_hw_disable(priv);
	enc28j60_lowpower(priv, true);
	netif_stop_queue(dev);
}

So a tx_work queued earlier by enc28j60_send_packet() can still run
during the cancel_work_sync() above, and enc28j60_hw_tx() has no
priv->hw_enable or netif_running() gate before it touches the chip:

drivers/net/ethernet/microchip/enc28j60.c:enc28j60_hw_tx() {
	enc28j60_packet_write(priv, priv->tx_skb->len, priv->tx_skb->data);
	...
	/* set TX request flag */
	locked_reg_bfset(priv, ECON1, ECON1_TXRTS);
}

Can that leave ECON1.TXRTS asserted after ECON2.PWRSV has already been
set?  enc28j60_lowpower() itself encodes the opposite ordering:

drivers/net/ethernet/microchip/enc28j60.c:enc28j60_lowpower() {
	nolock_reg_bfclr(priv, ECON1, ECON1_RXEN);
	poll_ready(priv, ESTAT, ESTAT_RXBUSY, 0);
	poll_ready(priv, ECON1, ECON1_TXRTS, 0);
	/* ECON2_VRPS was set during initialization */
	nolock_reg_bfset(priv, ECON2, ECON2_PWRSV);
}

With EIE zeroed by enc28j60_hw_disable() and the IRQ already freed, is
there anything left that can retire that transmit?  The only place that
clears TXRTS and wakes the queue is reachable solely from
enc28j60_irq():

drivers/net/ethernet/microchip/enc28j60.c:enc28j60_tx_clear() {
	locked_reg_bfclr(priv, ECON1, ECON1_TXRTS);
	netif_wake_queue(ndev);
}

The same missing quiesce also applies to the plain "ip link set ethX
down" case, where nothing cancels tx_work or setrx_work at all, so a
deferred transmit or an ERXFCON update can hit a powered-down chip, and
a late tx_work can arm TXRTS while a subsequent enc28j60_net_open() is
running the enc28j60_hw_init() soft reset and txfifo init (priv->lock is
only held per register access, not across that state transition).

Would it make sense to cancel the hardware-touching works while the
interface is still up, for instance in enc28j60_net_close() or in a
helper shared by close and remove, and/or re-quiesce the chip after the
cancels so both teardown paths follow the same rule?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260830-enc28j60-cancel-works-v1-1-4c40f17d401e%40cherr.cc