[PATCH net] nfc: pn533: fix use-after-free in pn533_recv_frame

[PATCH net] nfc: pn533: fix use-after-free in pn533_recv_frame

[Yinhao Hu]

pn533_recv_frame() checks dev->cmd and then dereferences it several
times to store the receive status and the response skb. In parallel the
command completion worker runs pn533_send_async_complete(), which frees
the same struct pn533_cmd and only clears dev->cmd after the free. With
fast receive/completion timing the receive path can therefore write to
an already freed command object.

Serialize all accesses to dev->cmd with a new cmd_state_lock spinlock:
pn533_recv_frame() now inspects and updates the current command while
holding the lock, the send paths publish and clear the pointer through
pn533_set_current_cmd(), and the completion worker detaches the command
under the lock before freeing it. Once the worker has detached the command,
a racing receive observes a NULL dev->cmd and no longer touches the freed
object.

  BUG: KASAN: slab-use-after-free in pn533_recv_frame+0x54f/0x670
  Write of size 4 at addr ffff888105566a14 by task kworker/1:3/126

  CPU: 1 UID: 0 PID: 126 Comm: kworker/1:3 Not tainted 7.1.0+ #8 PREEMPT
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
  Workqueue: events nfc_urelease_event_work
  Call Trace:
   <TASK>
   dump_stack_lvl+0x8c/0xb0
   print_report+0xd0/0x630
   kasan_report+0xce/0x100
   pn533_recv_frame+0x54f/0x670
   pn533_stop_poll+0xdc/0x150
   nfc_stop_poll+0xf2/0x1c0
   nfc_urelease_event_work+0x194/0x270
   process_one_work+0x701/0x1160
   worker_thread+0x5e6/0xd50
   kthread+0x373/0x4c0
   ret_from_fork+0x2e9/0x750
   ret_from_fork_asm+0x1a/0x30
   </TASK>

  Allocated by task 1744:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x8f/0xa0
   __kmalloc_cache_noprof+0x194/0x430
   __pn533_send_async+0x5c/0x410
   pn533_wq_rf+0xc6/0x160
   process_one_work+0x701/0x1160
   worker_thread+0x5e6/0xd50
   kthread+0x373/0x4c0
   ret_from_fork+0x2e9/0x750
   ret_from_fork_asm+0x1a/0x30

  Freed by task 1744:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   kasan_save_free_info+0x3b/0x60
   __kasan_slab_free+0x43/0x70
   kfree+0x17b/0x440
   pn533_wq_cmd_complete+0x24d/0x450
   process_one_work+0x701/0x1160
   worker_thread+0x5e6/0xd50
   kthread+0x373/0x4c0
   ret_from_fork+0x2e9/0x750
   ret_from_fork_asm+0x1a/0x30

Fixes: 9815c7cf22da ("NFC: pn533: Separate physical layer from the core implementation")
Signed-off-by: Yinhao Hu <dddddd@hust.edu.cn>
---
 drivers/nfc/pn533/pn533.c | 47 ++++++++++++++++++++++++++++++++-------
 drivers/nfc/pn533/pn533.h |  1 +
 2 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
index d7bdbc82e2ba..921e93a5f16f 100644
--- a/drivers/nfc/pn533/pn533.c
+++ b/drivers/nfc/pn533/pn533.c
@@ -394,12 +394,32 @@ static void pn533_build_cmd_frame(struct pn533 *dev, u8 cmd_code,
 	ops->tx_frame_finish(skb->data);
 }
 
+static void pn533_set_current_cmd(struct pn533 *dev, struct pn533_cmd *cmd)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->cmd_state_lock, flags);
+	dev->cmd = cmd;
+	spin_unlock_irqrestore(&dev->cmd_state_lock, flags);
+}
+
 static int pn533_send_async_complete(struct pn533 *dev)
 {
-	struct pn533_cmd *cmd = dev->cmd;
+	struct pn533_cmd *cmd;
 	struct sk_buff *resp;
+	unsigned long flags;
 	int status, rc = 0;
 
+	/*
+	 * Detach the current command before freeing it, so a concurrent
+	 * pn533_recv_frame() either observes a valid command under the lock
+	 * or a NULL dev->cmd and stops touching the freed object.
+	 */
+	spin_lock_irqsave(&dev->cmd_state_lock, flags);
+	cmd = dev->cmd;
+	dev->cmd = NULL;
+	spin_unlock_irqrestore(&dev->cmd_state_lock, flags);
+
 	if (!cmd) {
 		dev_dbg(dev->dev, "%s: cmd not set\n", __func__);
 		goto done;
@@ -430,7 +450,6 @@ static int pn533_send_async_complete(struct pn533 *dev)
 
 done:
 	kfree(cmd);
-	dev->cmd = NULL;
 	return rc;
 }
 
@@ -458,10 +477,10 @@ static int __pn533_send_async(struct pn533 *dev, u8 cmd_code,
 	mutex_lock(&dev->cmd_lock);
 
 	if (!dev->cmd_pending) {
-		dev->cmd = cmd;
+		pn533_set_current_cmd(dev, cmd);
 		rc = dev->phy_ops->send_frame(dev, req);
 		if (rc) {
-			dev->cmd = NULL;
+			pn533_set_current_cmd(dev, NULL);
 			goto error;
 		}
 
@@ -529,10 +548,10 @@ static int pn533_send_cmd_direct_async(struct pn533 *dev, u8 cmd_code,
 
 	pn533_build_cmd_frame(dev, cmd_code, req);
 
-	dev->cmd = cmd;
+	pn533_set_current_cmd(dev, cmd);
 	rc = dev->phy_ops->send_frame(dev, req);
 	if (rc < 0) {
-		dev->cmd = NULL;
+		pn533_set_current_cmd(dev, NULL);
 		kfree(cmd);
 	}
 
@@ -569,10 +588,10 @@ static void pn533_wq_cmd(struct work_struct *work)
 
 	mutex_unlock(&dev->cmd_lock);
 
-	dev->cmd = cmd;
+	pn533_set_current_cmd(dev, cmd);
 	rc = dev->phy_ops->send_frame(dev, cmd->req);
 	if (rc < 0) {
-		dev->cmd = NULL;
+		pn533_set_current_cmd(dev, NULL);
 		dev_kfree_skb(cmd->req);
 		kfree(cmd);
 		return;
@@ -2165,6 +2184,15 @@ static int pn533_data_exchange_complete(struct pn533 *dev, void *_arg,
  */
 void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
 {
+	unsigned long flags;
+
+	/*
+	 * Hold cmd_state_lock across the whole receive path so the current
+	 * command cannot be freed by pn533_send_async_complete() between the
+	 * dev->cmd check and the stores into it.
+	 */
+	spin_lock_irqsave(&dev->cmd_state_lock, flags);
+
 	if (!dev->cmd)
 		goto sched_wq;
 
@@ -2182,6 +2210,7 @@ void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
 
 	if (pn533_rx_frame_is_ack(skb->data)) {
 		dev_dbg(dev->dev, "%s: Received ACK frame\n", __func__);
+		spin_unlock_irqrestore(&dev->cmd_state_lock, flags);
 		dev_kfree_skb(skb);
 		return;
 	}
@@ -2200,6 +2229,7 @@ void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
 	dev->cmd->resp = skb;
 
 sched_wq:
+	spin_unlock_irqrestore(&dev->cmd_state_lock, flags);
 	queue_work(dev->wq, &dev->cmd_complete_work);
 }
 EXPORT_SYMBOL(pn533_recv_frame);
@@ -2760,6 +2790,7 @@ struct pn533 *pn53x_common_init(u32 device_type,
 	priv->device_type = device_type;
 
 	mutex_init(&priv->cmd_lock);
+	spin_lock_init(&priv->cmd_state_lock);
 
 	INIT_WORK(&priv->cmd_work, pn533_wq_cmd);
 	INIT_WORK(&priv->cmd_complete_work, pn533_wq_cmd_complete);
diff --git a/drivers/nfc/pn533/pn533.h b/drivers/nfc/pn533/pn533.h
index 09e35b8693f5..8b009b2318d0 100644
--- a/drivers/nfc/pn533/pn533.h
+++ b/drivers/nfc/pn533/pn533.h
@@ -153,6 +153,7 @@ struct pn533 {
 	struct pn533_cmd *cmd;
 	u8 cmd_pending;
 	struct mutex cmd_lock;  /* protects cmd queue */
+	spinlock_t cmd_state_lock;  /* protects dev->cmd lifetime */
 
 	void *cmd_complete_mi_arg;
 	void *cmd_complete_dep_arg;
-- 
2.43.0

Re: [PATCH net] nfc: pn533: fix use-after-free in pn533_recv_frame

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
nfc: pn533: fix use-after-free in pn533_recv_frame

This commit adds a spinlock cmd_state_lock to serialize access to dev->cmd 
in pn533_recv_frame() and the command completion worker. It aims to prevent 
a use-after-free when a command is detached and freed while being 
concurrently received.

> diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> index d7bdbc82e2ba2..921e93a5f16f5 100644
> --- a/drivers/nfc/pn533/pn533.c
> +++ b/drivers/nfc/pn533/pn533.c

[ ... ]

> @@ -458,10 +477,10 @@ static int __pn533_send_async(struct pn533 *dev, u8 cmd_code,
>  	mutex_lock(&dev->cmd_lock);
>  
>  	if (!dev->cmd_pending) {
> -		dev->cmd = cmd;
> +		pn533_set_current_cmd(dev, cmd);
>  		rc = dev->phy_ops->send_frame(dev, req);
>  		if (rc) {
> -			dev->cmd = NULL;
> +			pn533_set_current_cmd(dev, NULL);

[Severity: High]
This is a pre-existing issue, but if dev->phy_ops->send_frame() fails, it
jumps to the error label and frees cmd. If a concurrent receive scheduled 
the complete work, could that work also free cmd, causing a double free?

>  			goto error;
>  		}

[ ... ]

> @@ -569,10 +588,10 @@ static void pn533_wq_cmd(struct work_struct *work)
>  
>  	mutex_unlock(&dev->cmd_lock);
>  
> -	dev->cmd = cmd;
> +	pn533_set_current_cmd(dev, cmd);
>  	rc = dev->phy_ops->send_frame(dev, cmd->req);
>  	if (rc < 0) {
> -		dev->cmd = NULL;
> +		pn533_set_current_cmd(dev, NULL);

[Severity: High]
This isn't a bug introduced by this patch, but if dev->phy_ops->send_frame() 
fails in pn533_wq_cmd(), cmd is freed. Does a race condition exist here 
similar to __pn533_send_async(), where concurrent complete work could also 
free cmd?

[Severity: High]
Additionally, this is also a pre-existing issue, but does this error path 
drop the command without calling complete_cb and without clearing 
dev->cmd_pending? Could this leave the command queue permanently hung 
upon transient bus errors?

>  		dev_kfree_skb(cmd->req);
>  		kfree(cmd);
>  		return;
>  	}

[ ... ]

> @@ -2165,6 +2184,15 @@ static int pn533_data_exchange_complete(struct pn533 *dev, void *_arg,
>   */
>  void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
>  {
> +	unsigned long flags;
> +
> +	/*
> +	 * Hold cmd_state_lock across the whole receive path so the current
> +	 * command cannot be freed by pn533_send_async_complete() between the
> +	 * dev->cmd check and the stores into it.
> +	 */
> +	spin_lock_irqsave(&dev->cmd_state_lock, flags);
> +
>  	if (!dev->cmd)
>  		goto sched_wq;

[Severity: Medium]
This isn't a bug introduced by this patch, but if there is no active command 
(!dev->cmd), this jumps to sched_wq without freeing the skb. Does this leak 
the skb if an unexpected frame arrives?

>  
>  	dev->cmd->status = status;

[ ... ]

> @@ -2182,6 +2210,7 @@ void pn533_recv_frame(struct pn533 *dev, struct sk_buff *skb, int status)
>  
>  	if (pn533_rx_frame_is_ack(skb->data)) {
>  		dev_dbg(dev->dev, "%s: Received ACK frame\n", __func__);
> +		spin_unlock_irqrestore(&dev->cmd_state_lock, flags);
>  		dev_kfree_skb(skb);
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but does pn533_rx_frame_is_ack() safely read 
from skb->data here? Since there doesn't appear to be a check for skb->len 
before calling it, could this result in an out-of-bounds read if a malformed 
or short frame is received?