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

[netdev-bot+sashiko@kernel.org]

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