Skip to contentSkip to conversation

nfc: fix leaks and races surfaced by NIPA

PATCH Applied Jakub Kicinski 4 participants 13 messages Mar 3 – Mar 24

Subsystems (3)
lore
Share & tools
lore

Jakub Kicinski

author maintainer
#0 lore raw

cover nfc: fix leaks and races surfaced by NIPA

I recently added the nci test to NIPA. Somewhat surprisingly it runs without much settup but hits kmemleaks fairly often. Fix a handful of issues to make the test pass in a stable way.

Jakub Kicinski (5):
nfc: nci: free skb on nci_transceive early error paths
nfc: digital: free skb on digital_in_send error paths
nfc: nci: complete pending data exchange on device close
nfc: nci: clear NCI_DATA_EXCHANGE before calling completion callback
nfc: rawsock: cancel tx_work before socket teardown

 net/nfc/digital_core.c |  8 ++++++--
 net/nfc/nci/core.c     | 18 ++++++++++++++++--
 net/nfc/nci/data.c     | 12 ++++++++----
 net/nfc/rawsock.c      | 11 +++++++++++
 4 files changed, 41 insertions(+), 8 deletions(-)
Signature · 1 line
2.53.0
author maintainer
#1 lore raw

PATCH 1/5 nfc: nci: free skb on nci_transceive early error paths

nci_transceive() takes ownership of the skb passed by the caller, but the -EPROTO, -EINVAL, and -EBUSY error paths return without freeing it.

Due to issues clearing NCI_DATA_EXCHANGE fixed by subsequent changes the nci/nci_dev selftest hits the error path occasionally in NIPA, and kmemleak detects leaks:

unreferenced object 0xff11000015ce6a40 (size 640):
comm "nci_dev", pid 3954, jiffies 4295441246
hex dump (first 32 bytes):
6b 6b 6b 6b 00 a4 00 0c 02 e1 03 6b 6b 6b 6b 6b kkkk.......kkkkk 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
backtrace (crc 7c40cc2a):

    kmem_cache_alloc_node_noprof+0x492/0x630
    __alloc_skb+0x11e/0x5f0
    alloc_skb_with_frags+0xc6/0x8f0
    sock_alloc_send_pskb+0x326/0x3f0
    nfc_alloc_send_skb+0x94/0x1d0
    rawsock_sendmsg+0x162/0x4c0
    do_syscall_64+0x117/0xfc0
Fixes 6a2968aaf50c ("NFC: basic NCI protocol implementation")
---
 net/nfc/nci/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
net/nfc/nci/core.c +7 -2
index 6e9b76e2cc56..40fc397858ce 100644
@@ -1035,18 +1035,23 @@ static int nci_transceive(struct nfc_dev *nfc_dev, struct nfc_target *target,
 	struct nci_conn_info *conn_info;
 
 	conn_info = ndev->rf_conn_info;
-	if (!conn_info)
+	if (!conn_info) {
+		kfree_skb(skb);
 		return -EPROTO;
+	}
 
 	pr_debug("target_idx %d, len %d\n", target->idx, skb->len);
 
 	if (!ndev->target_active_prot) {
 		pr_err("unable to exchange data, no active target\n");
+		kfree_skb(skb);
 		return -EINVAL;
 	}
 
-	if (test_and_set_bit(NCI_DATA_EXCHANGE, &ndev->flags))
+	if (test_and_set_bit(NCI_DATA_EXCHANGE, &ndev->flags)) {
+		kfree_skb(skb);
 		return -EBUSY;
+	}
 
 	/* store cb and context to be used on receiving data */
 	conn_info->data_exchange_cb = cb;
Signature · 1 line
2.53.0
Reviewed-by on v1 1/5
#2 lore raw

Re: [PATCH net 1/5] nfc: nci: free skb on nci_transceive early error paths

Quoted 22 lines · Jakub Kicinski

nci_transceive() takes ownership of the skb passed by the caller, but the -EPROTO, -EINVAL, and -EBUSY error paths return without freeing it.

Due to issues clearing NCI_DATA_EXCHANGE fixed by subsequent changes the nci/nci_dev selftest hits the error path occasionally in NIPA, and kmemleak detects leaks:

unreferenced object 0xff11000015ce6a40 (size 640):
comm "nci_dev", pid 3954, jiffies 4295441246
hex dump (first 32 bytes):
6b 6b 6b 6b 00 a4 00 0c 02 e1 03 6b 6b 6b 6b 6b kkkk.......kkkkk 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
backtrace (crc 7c40cc2a):

    kmem_cache_alloc_node_noprof+0x492/0x630
    __alloc_skb+0x11e/0x5f0
    alloc_skb_with_frags+0xc6/0x8f0
    sock_alloc_send_pskb+0x326/0x3f0
    nfc_alloc_send_skb+0x94/0x1d0
    rawsock_sendmsg+0x162/0x4c0
    do_syscall_64+0x117/0xfc0
Quoted 6 lines
Fixes 6a2968aaf50c ("NFC: basic NCI protocol implementation")
---
 net/nfc/nci/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
author maintainer
#3 lore raw

PATCH 2/5 nfc: digital: free skb on digital_in_send error paths

In reply to Jakub Kicinski

digital_in_send() takes ownership of the skb passed by the caller (nfc_data_exchange), make sure it's freed on all error paths.

Found looking around the real driver for similar bugs to the one just fixed in nci.

Fixes 7d0911c02fa2 ("NFC: digital: Add NFC-DEP Initiator-side send/receive support")
---
 net/nfc/digital_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
net/nfc/digital_core.c +6 -2
index 3670bb33732e..7cb1e6aaae90 100644
@@ -707,8 +707,10 @@ static int digital_in_send(struct nfc_dev *nfc_dev, struct nfc_target *target,
 	int rc;
 
 	data_exch = kzalloc_obj(*data_exch);
-	if (!data_exch)
+	if (!data_exch) {
+		kfree_skb(skb);
 		return -ENOMEM;
+	}
 
 	data_exch->cb = cb;
 	data_exch->cb_context = cb_context;
@@ -731,8 +733,10 @@ static int digital_in_send(struct nfc_dev *nfc_dev, struct nfc_target *target,
 				 data_exch);
 
 exit:
-	if (rc)
+	if (rc) {
+		kfree_skb(skb);
 		kfree(data_exch);
+	}
 
 	return rc;
 }
Signature · 1 line
2.53.0
Reviewed-by on v1 2/5
#4 lore raw

Re: [PATCH net 2/5] nfc: digital: free skb on digital_in_send error paths

Quoted 12 lines · Jakub Kicinski

digital_in_send() takes ownership of the skb passed by the caller (nfc_data_exchange), make sure it's freed on all error paths.

Found looking around the real driver for similar bugs to the one just fixed in nci.

Fixes 7d0911c02fa2 ("NFC: digital: Add NFC-DEP Initiator-side send/receive support")
---
 net/nfc/digital_core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
author maintainer
#5 lore raw

PATCH 3/5 nfc: nci: complete pending data exchange on device close

In reply to Jakub Kicinski

In nci_close_device(), complete any pending data exchange before closing. The data exchange callback (e.g. rawsock_data_exchange_complete) holds a socket reference.

NIPA occasionally hits this leak:

unreferenced object 0xff1100000f435000 (size 2048):
comm "nci_dev", pid 3954, jiffies 4295441245 hex dump (first 32 bytes):

    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    27 00 01 40 00 00 00 00 00 00 00 00 00 00 00 00  '..@............
  backtrace (crc ec2b3c5):
    __kmalloc_noprof+0x4db/0x730
    sk_prot_alloc.isra.0+0xe4/0x1d0
    sk_alloc+0x36/0x760
    rawsock_create+0xd1/0x540
    nfc_sock_create+0x11f/0x280
    __sock_create+0x22d/0x630
    __sys_socket+0x115/0x1d0
    __x64_sys_socket+0x72/0xd0
    do_syscall_64+0x117/0xfc0
    entry_SYSCALL_64_after_hwframe+0x4b/0x53
Fixes 38f04c6b1b68 ("NFC: protect nci_data_exchange transactions")
---
 net/nfc/nci/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)
net/nfc/nci/core.c +9 -0
index 40fc397858ce..f8c0bab2ec07 100644
@@ -567,6 +567,10 @@ static int nci_close_device(struct nci_dev *ndev)
 		flush_workqueue(ndev->cmd_wq);
 		timer_delete_sync(&ndev->cmd_timer);
 		timer_delete_sync(&ndev->data_timer);
+		if (test_bit(NCI_DATA_EXCHANGE, &ndev->flags))
+			nci_data_exchange_complete(ndev, NULL,
+						   ndev->cur_conn_id,
+						   -ENODEV);
 		mutex_unlock(&ndev->req_lock);
 		return 0;
 	}
@@ -598,6 +602,11 @@ static int nci_close_device(struct nci_dev *ndev)
 	flush_workqueue(ndev->cmd_wq);
 
 	timer_delete_sync(&ndev->cmd_timer);
+	timer_delete_sync(&ndev->data_timer);
+
+	if (test_bit(NCI_DATA_EXCHANGE, &ndev->flags))
+		nci_data_exchange_complete(ndev, NULL, ndev->cur_conn_id,
+					   -ENODEV);
 
 	/* Clear flags except NCI_UNREG */
 	ndev->flags &= BIT(NCI_UNREG);
Signature · 1 line
2.53.0
Reviewed-by on v1 3/5
#6 lore raw

Re: [PATCH net 3/5] nfc: nci: complete pending data exchange on device close

Quoted 23 lines · Jakub Kicinski

In nci_close_device(), complete any pending data exchange before closing. The data exchange callback (e.g. rawsock_data_exchange_complete) holds a socket reference.

NIPA occasionally hits this leak:

unreferenced object 0xff1100000f435000 (size 2048):
comm "nci_dev", pid 3954, jiffies 4295441245 hex dump (first 32 bytes):

    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
    27 00 01 40 00 00 00 00 00 00 00 00 00 00 00 00  '..@............
  backtrace (crc ec2b3c5):
    __kmalloc_noprof+0x4db/0x730
    sk_prot_alloc.isra.0+0xe4/0x1d0
    sk_alloc+0x36/0x760
    rawsock_create+0xd1/0x540
    nfc_sock_create+0x11f/0x280
    __sock_create+0x22d/0x630
    __sys_socket+0x115/0x1d0
    __x64_sys_socket+0x72/0xd0
    do_syscall_64+0x117/0xfc0
    entry_SYSCALL_64_after_hwframe+0x4b/0x53
Quoted 5 lines
Fixes 38f04c6b1b68 ("NFC: protect nci_data_exchange transactions")
---
 net/nfc/nci/core.c | 9 +++++++++
 1 file changed, 9 insertions(+)
author maintainer
#7 lore raw

PATCH 4/5 nfc: nci: clear NCI_DATA_EXCHANGE before calling completion callback

In reply to Jakub Kicinski

Move clear_bit(NCI_DATA_EXCHANGE) before invoking the data exchange callback in nci_data_exchange_complete().

The callback (e.g. rawsock_data_exchange_complete) may immediately schedule another data exchange via schedule_work(tx_work). On a multi-CPU system, tx_work can run and reach nci_transceive() before the current nci_data_exchange_complete() clears the flag, causing test_and_set_bit(NCI_DATA_EXCHANGE) to return -EBUSY and the new transfer to fail.

This causes intermittent flakes in nci/nci_dev in NIPA:

# # RUN NCI.NCI1_0.t4t_tag_read ...
# # t4t_tag_read: Test terminated by timeout
# # FAIL NCI.NCI1_0.t4t_tag_read

  # not ok 3 NCI.NCI1_0.t4t_tag_read
Fixes 38f04c6b1b68 ("NFC: protect nci_data_exchange transactions")
---
 net/nfc/nci/data.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
net/nfc/nci/data.c +8 -4
index 78f4131af3cf..efb001b41aa8 100644
@@ -33,7 +33,8 @@ void nci_data_exchange_complete(struct nci_dev *ndev, struct sk_buff *skb,
 	conn_info = nci_get_conn_info_by_conn_id(ndev, conn_id);
 	if (!conn_info) {
 		kfree_skb(skb);
-		goto exit;
+		clear_bit(NCI_DATA_EXCHANGE, &ndev->flags);
+		return;
 	}
 
 	cb = conn_info->data_exchange_cb;
@@ -45,6 +46,12 @@ void nci_data_exchange_complete(struct nci_dev *ndev, struct sk_buff *skb,
 	timer_delete_sync(&ndev->data_timer);
 	clear_bit(NCI_DATA_EXCHANGE_TO, &ndev->flags);
 
+	/* Mark the exchange as done before calling the callback.
+	 * The callback (e.g. rawsock_data_exchange_complete) may
+	 * want to immediately queue another data exchange.
+         */
+	clear_bit(NCI_DATA_EXCHANGE, &ndev->flags);
+
 	if (cb) {
 		/* forward skb to nfc core */
 		cb(cb_context, skb, err);
@@ -54,9 +61,6 @@ void nci_data_exchange_complete(struct nci_dev *ndev, struct sk_buff *skb,
 		/* no waiting callback, free skb */
 		kfree_skb(skb);
 	}
-
-exit:
-	clear_bit(NCI_DATA_EXCHANGE, &ndev->flags);
 }
 
 /* ----------------- NCI TX Data ----------------- */
Signature · 1 line
2.53.0
Reviewed-by on v1 4/5
#8 lore raw

Re: [PATCH net 4/5] nfc: nci: clear NCI_DATA_EXCHANGE before calling completion callback

Quoted 15 lines · Jakub Kicinski

Move clear_bit(NCI_DATA_EXCHANGE) before invoking the data exchange callback in nci_data_exchange_complete().

The callback (e.g. rawsock_data_exchange_complete) may immediately schedule another data exchange via schedule_work(tx_work). On a multi-CPU system, tx_work can run and reach nci_transceive() before the current nci_data_exchange_complete() clears the flag, causing test_and_set_bit(NCI_DATA_EXCHANGE) to return -EBUSY and the new transfer to fail.

This causes intermittent flakes in nci/nci_dev in NIPA:

# # RUN NCI.NCI1_0.t4t_tag_read ...
# # t4t_tag_read: Test terminated by timeout
# # FAIL NCI.NCI1_0.t4t_tag_read

Quoted 7 lines
  # not ok 3 NCI.NCI1_0.t4t_tag_read
Fixes 38f04c6b1b68 ("NFC: protect nci_data_exchange transactions")
---
 net/nfc/nci/data.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
author maintainer
#9 lore raw

PATCH 5/5 nfc: rawsock: cancel tx_work before socket teardown

In reply to Jakub Kicinski

In rawsock_release(), cancel any pending tx_work and purge the write queue before orphaning the socket. rawsock_tx_work runs on the system workqueue and calls nfc_data_exchange which dereferences the NCI device. Without synchronization, tx_work can race with socket and device teardown when a process is killed (e.g. by SIGKILL), leading to use-after-free or leaked references.

Set SEND_SHUTDOWN first so that if tx_work is already running it will see the flag and skip transmitting, then use cancel_work_sync to wait for any in-progress execution to finish, and finally purge any remaining queued skbs.

Fixes 23b7869c0fd0 ("NFC: add the NFC socket raw protocol")
---
 net/nfc/rawsock.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
net/nfc/rawsock.c +11 -0
index b049022399ae..f7d7a599fade 100644
@@ -67,6 +67,17 @@ static int rawsock_release(struct socket *sock)
 	if (sock->type == SOCK_RAW)
 		nfc_sock_unlink(&raw_sk_list, sk);
 
+	if (sk->sk_state == TCP_ESTABLISHED) {
+		/* Prevent rawsock_tx_work from starting new transmits and
+		 * wait for any in-progress work to finish.  This must happen
+		 * before the socket is orphaned to avoid a race where
+		 * rawsock_tx_work runs after the NCI device has been freed.
+		 */
+		sk->sk_shutdown |= SEND_SHUTDOWN;
+		cancel_work_sync(&nfc_rawsock(sk)->tx_work);
+		rawsock_write_queue_purge(sk);
+	}
+
 	sock_orphan(sk);
 	sock_put(sk);
 
Signature · 1 line
2.53.0
Reviewed-by on v1 5/5
#10 lore raw

Re: [PATCH net 5/5] nfc: rawsock: cancel tx_work before socket teardown

Quoted 12 lines · Jakub Kicinski

In rawsock_release(), cancel any pending tx_work and purge the write queue before orphaning the socket. rawsock_tx_work runs on the system workqueue and calls nfc_data_exchange which dereferences the NCI device. Without synchronization, tx_work can race with socket and device teardown when a process is killed (e.g. by SIGKILL), leading to use-after-free or leaked references.

Set SEND_SHUTDOWN first so that if tx_work is already running it will see the flag and skip transmitting, then use cancel_work_sync to wait for any in-progress execution to finish, and finally purge any remaining queued skbs.

Quoted 6 lines
Fixes 23b7869c0fd0 ("NFC: add the NFC socket raw protocol")
---
 net/nfc/rawsock.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
#11 lore raw

Re: [PATCH net 5/5] nfc: rawsock: cancel tx_work before socket teardown

In reply to Jakub Kicinski

Hi,

Quoted 27 lines · Jakub Kicinski

In rawsock_release(), cancel any pending tx_work and purge the write queue before orphaning the socket. rawsock_tx_work runs on the system workqueue and calls nfc_data_exchange which dereferences the NCI device. Without synchronization, tx_work can race with socket and device teardown when a process is killed (e.g. by SIGKILL), leading to use-after-free or leaked references.

Set SEND_SHUTDOWN first so that if tx_work is already running it will see the flag and skip transmitting, then use cancel_work_sync to wait for any in-progress execution to finish, and finally purge any remaining queued skbs.

Fixes 23b7869c0fd0 ("NFC: add the NFC socket raw protocol")
---
 net/nfc/rawsock.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
net/nfc/rawsock.c +1 -0
index b049022399ae..f7d7a599fade 100644
@@ -67,6 +67,17 @@ static int rawsock_release(struct socket *sock)
 	if (sock->type == SOCK_RAW)
 		nfc_sock_unlink(&raw_sk_list, sk);
 
+	if (sk->sk_state == TCP_ESTABLISHED) {

net/nfc/rawsock.c static int rawsock_release(struct socket *sock) · line 80

Quoted 10 lines
net/nfc/rawsock.c +10 -0
+		/* Prevent rawsock_tx_work from starting new transmits and
+		 * wait for any in-progress work to finish.  This must happen
+		 * before the socket is orphaned to avoid a race where
+		 * rawsock_tx_work runs after the NCI device has been freed.
+		 */
+		sk->sk_shutdown |= SEND_SHUTDOWN;
+		cancel_work_sync(&nfc_rawsock(sk)->tx_work);
+		rawsock_write_queue_purge(sk);
+	}
+

Google's AI agent had the following fedback on this patch:

Does cancel_work_sync() properly wait for the asynchronous nfc_data_exchange() to complete?

Since rawsock_tx_work() only initiates the exchange and then returns, the work item is no longer running or pending while the exchange is in flight. This means cancel_work_sync() might return immediately.

net/nfc/rawsock.c static int rawsock_release(struct socket *sock) · line 78

Quoted 1 line
net/nfc/rawsock.c +1 -0
+		rawsock_write_queue_purge(sk);

If the hardware completes the data exchange after cancel_work_sync() has returned, but before rawsock_write_queue_purge() empties the write queue, rawsock_data_exchange_complete() will see a non-empty queue and call schedule_work().

Because cancel_work_sync() has already finished, this newly scheduled tx_work could execute after rawsock_release() has freed the socket. Can this lead to a use-after-free?

On top of that, I don't see how the this patch change would prevent the problem it tries to fix if tx_work is running and has proceeded beyond the SEND_SHUTDOWN check.

My apologies if this is all just noise. If so, please let me know what is wrong with the analysis to help improve the agent.

Thanks,
Guenter

#12 lore raw
In reply to Jakub Kicinski
This series was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 4 lines · On Tue, 3 Mar 2026 08:23:40 -0800 you wrote:

I recently added the nci test to NIPA. Somewhat surprisingly it runs without much settup but hits kmemleaks fairly often. Fix a handful of issues to make the test pass in a stable way.

Quoted 8 lines

Jakub Kicinski (5):
nfc: nci: free skb on nci_transceive early error paths
nfc: digital: free skb on digital_in_send error paths
nfc: nci: complete pending data exchange on device close
nfc: nci: clear NCI_DATA_EXCHANGE before calling completion callback
nfc: rawsock: cancel tx_work before socket teardown

[...]

Here is the summary with links:
- [net,1/5] nfc: nci: free skb on nci_transceive early error paths

    https://git.kernel.org/netdev/net/c/7bd4b0c4779f
  - [net,2/5] nfc: digital: free skb on digital_in_send error paths
    https://git.kernel.org/netdev/net/c/d42449d2c17c
  - [net,3/5] nfc: nci: complete pending data exchange on device close
    https://git.kernel.org/netdev/net/c/66083581945b
  - [net,4/5] nfc: nci: clear NCI_DATA_EXCHANGE before calling completion callback
    https://git.kernel.org/netdev/net/c/0efdc02f4f6d
  - [net,5/5] nfc: rawsock: cancel tx_work before socket teardown
    https://git.kernel.org/netdev/net/c/d793458c45df

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html