Re: [PATCH net] nfc: pn533: fix use-after-free in pn533_recv_frame
From: Simon Horman <horms@kernel.org>
Date: 2026-07-08 11:09:00
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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; }
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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; }
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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;
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?