[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 <ddd...@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