[PATCH] ieee802154: cc2520: fix FIFOP work use-after-free

[PATCH] ieee802154: cc2520: fix FIFOP work use-after-free

[Fan Wu]

The FIFOP interrupt handler queues cc2520_fifop_irqwork.  On
removal, cc2520_remove() only flushes the work.  The devm-managed
FIFOP IRQ remains active until after ->remove() returns and can queue
the work again after that flush, allowing it to run after the private
data is released.

Release the FIFOP IRQ and cancel the work before unregistering and
freeing the hardware.  Keep the SFD IRQ active while
ieee802154_unregister_hw() flushes the mac802154 workqueue: synchronous
TX waits in cc2520_tx() for the completion signalled by the SFD handler.
Release the SFD IRQ afterwards.  Destroy buffer_mutex last, since
unregistering can invoke the driver's stop callback, which uses it.

Install the SFD IRQ before cc2520_register(), so it is available when
the netdev becomes visible.  Install the FIFOP IRQ afterwards, so a
registration failure cannot schedule RX work while its hardware is
being released.  Move ieee802154_free_hw() to the probe cleanup.

Found by an in-house static analysis tool.

Fixes: 0da6bc8cc341 ("ieee802154: cc2520: adds driver for TI CC2520 radio")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/net/ieee802154/cc2520.c | 51 ++++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
index 2b7034193..464e896e3 100644
--- a/drivers/net/ieee802154/cc2520.c
+++ b/drivers/net/ieee802154/cc2520.c
@@ -206,6 +206,8 @@ struct cc2520_private {
 	bool is_tx;			/* Flag for sync b/w Tx and Rx */
 	bool amplified;			/* Flag for CC2591 */
 	struct gpio_desc *fifo_pin;	/* FIFO GPIO pin number */
+	int fifop_irq;
+	int sfd_irq;
 	struct work_struct fifop_irqwork;/* Workqueue for FIFOP */
 	spinlock_t lock;		/* Lock for is_tx*/
 	struct completion tx_complete;	/* Work completion for Tx */
@@ -857,12 +859,10 @@ static int cc2520_register(struct cc2520_private *priv)
 	dev_vdbg(&priv->spi->dev, "registered cc2520\n");
 	ret = ieee802154_register_hw(priv->hw);
 	if (ret)
-		goto err_free_device;
+		goto err_ret;
 
 	return 0;
 
-err_free_device:
-	ieee802154_free_hw(priv->hw);
 err_ret:
 	return ret;
 }
@@ -1116,21 +1116,10 @@ static int cc2520_probe(struct spi_device *spi)
 	if (ret)
 		goto err_hw_init;
 
-	/* Set up fifop interrupt */
+	/* SFD completes synchronous TX; install before cc2520_register(). */
+	priv->sfd_irq = gpiod_to_irq(sfd);
 	ret = devm_request_irq(&spi->dev,
-			       gpiod_to_irq(fifop),
-			       cc2520_fifop_isr,
-			       IRQF_TRIGGER_RISING,
-			       dev_name(&spi->dev),
-			       priv);
-	if (ret) {
-		dev_err(&spi->dev, "could not get fifop irq\n");
-		goto err_hw_init;
-	}
-
-	/* Set up sfd interrupt */
-	ret = devm_request_irq(&spi->dev,
-			       gpiod_to_irq(sfd),
+			       priv->sfd_irq,
 			       cc2520_sfd_isr,
 			       IRQF_TRIGGER_FALLING,
 			       dev_name(&spi->dev),
@@ -1142,13 +1131,31 @@ static int cc2520_probe(struct spi_device *spi)
 
 	ret = cc2520_register(priv);
 	if (ret)
-		goto err_hw_init;
+		goto err_free_sfd;
+
+	/* FIFOP arms the RX work; install after cc2520_register(). */
+	priv->fifop_irq = gpiod_to_irq(fifop);
+	ret = devm_request_irq(&spi->dev,
+			       priv->fifop_irq,
+			       cc2520_fifop_isr,
+			       IRQF_TRIGGER_RISING,
+			       dev_name(&spi->dev),
+			       priv);
+	if (ret) {
+		dev_err(&spi->dev, "could not get fifop irq\n");
+		goto err_unregister;
+	}
 
 	return 0;
 
+err_unregister:
+	ieee802154_unregister_hw(priv->hw);
+err_free_sfd:
+	devm_free_irq(&spi->dev, priv->sfd_irq, priv);
+	if (priv->hw)
+		ieee802154_free_hw(priv->hw);
 err_hw_init:
 	mutex_destroy(&priv->buffer_mutex);
-	flush_work(&priv->fifop_irqwork);
 	return ret;
 }
 
@@ -1156,11 +1163,13 @@ static void cc2520_remove(struct spi_device *spi)
 {
 	struct cc2520_private *priv = spi_get_drvdata(spi);
 
-	mutex_destroy(&priv->buffer_mutex);
-	flush_work(&priv->fifop_irqwork);
+	devm_free_irq(&spi->dev, priv->fifop_irq, priv);
+	cancel_work_sync(&priv->fifop_irqwork);
 
 	ieee802154_unregister_hw(priv->hw);
+	devm_free_irq(&spi->dev, priv->sfd_irq, priv);
 	ieee802154_free_hw(priv->hw);
+	mutex_destroy(&priv->buffer_mutex);
 }
 
 static const struct spi_device_id cc2520_ids[] = {

Re: [PATCH] ieee802154: cc2520: fix FIFOP work use-after-free

[Miquel Raynal]

Hi Fan,

On 06/08/2026 at 05:03:28 GMT, Fan Wu <fanwu01@zju.edu.cn> wrote:

> The FIFOP interrupt handler queues cc2520_fifop_irqwork.  On
> removal, cc2520_remove() only flushes the work.  The devm-managed
> FIFOP IRQ remains active until after ->remove() returns and can queue
> the work again after that flush, allowing it to run after the private
> data is released.
>
> Release the FIFOP IRQ and cancel the work before unregistering and
> freeing the hardware.  Keep the SFD IRQ active while
> ieee802154_unregister_hw() flushes the mac802154 workqueue: synchronous
> TX waits in cc2520_tx() for the completion signalled by the SFD handler.
> Release the SFD IRQ afterwards.  Destroy buffer_mutex last, since
> unregistering can invoke the driver's stop callback, which uses it.
>
> Install the SFD IRQ before cc2520_register(), so it is available when
> the netdev becomes visible.  Install the FIFOP IRQ afterwards, so a
> registration failure cannot schedule RX work while its hardware is
> being released.  Move ieee802154_free_hw() to the probe cleanup.
>
> Found by an in-house static analysis tool.
>
> Fixes: 0da6bc8cc341 ("ieee802154: cc2520: adds driver for TI CC2520 radio")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>

While I may agree on the problem, I don't like the solution. It is
overly complex. If the problem is the fact that we might queue works,
what about disable_work[_sync]() ?

Thanks,
Miquèl

Re: [PATCH] ieee802154: cc2520: fix FIFOP work use-after-free

[Fan Wu]

 Thanks, this is a much simpler solution,  I will respin accordingly.

> On Aug 7, 2026, at 20:39, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> Hi Fan,
> 
> While I may agree on the problem, I don't like the solution. It is
> overly complex. If the problem is the fact that we might queue works,
> what about disable_work[_sync]() ?
> 
> Thanks,
> Miquèl