[PATCH v3 1/1] nfc: llcp: Fix race condition in accept_queue lifecycle

[PATCH v3 1/1] nfc: llcp: Fix race condition in accept_queue lifecycle

[Lee Jones]

In nfc_llcp_socket_release(), sockets and listener accept queues are
walked under the local sockets rwlock and bh_lock_sock().  However,
bh_lock_sock() does not synchronise against process-context lock_sock()
held by nfc_llcp_accept_dequeue() during accept().  Because
socket_release() does not check sock_owned_by_user(), both paths can
concurrently unlink and release the same child socket, resulting in
use-after-free or a NULL pointer dereference of child->parent in
nfc_llcp_accept_unlink().

Fix this synchronisation race by having nfc_llcp_socket_release() use
process-context lock_sock() instead of bh_lock_sock():

1. Pop sockets from the local sockets list under the write lock using
   nfc_llcp_sock_list_pop() so lock_sock() can be acquired without
   holding the rwlock.

2. Because lock_sock() can sleep, defer the final release of the
   nfc_llcp_local structure to a workqueue (release_work).  This avoids
   a sleeping-in-atomic bug when the last local reference is dropped
   from softirq context.  Additionally, hold a single device reference
   on local from registration until final destruction.

3. In nfc_llcp_local_get(), use kref_get_unless_zero() to prevent
   resurrecting a local object whose teardown has been scheduled.

4. In llcp_sock_accept(), verify that the listener socket state is still
   LLCP_LISTEN after waking from schedule_timeout() to prevent hangs if
   the listener is closed concurrently.

5. When unlinking unaccepted child sockets during listener release,
   unlink them from local->sockets, call sock_orphan(), and drop their
   initial sk_alloc creation reference via sock_put().

6. Make nfc_llcp_accept_unlink() idempotent by guarding parent access with
   a NULL check.

Fixes: 50b78b2a6500 ("NFC: Fix sleeping in atomic when releasing socket")
Signed-off-by: Lee Jones <lee@kernel.org>
---

v1 -> v2:
- Defer local release to dedicated workqueue (llcp_wq) to avoid sleeping in atomic
- Drain and destroy llcp_wq on module unload to prevent module exit race
- Drop initial sk_alloc creation ref on unaccepted child sockets across all teardown paths
- Use kref_get_unless_zero() in local_get to prevent resurrecting dying local objects
- Check listener socket state after waking in llcp_sock_accept() to avoid hangs
- Standardise Parent (0) -> Child (1) lockdep subclass nesting

v2 -> v3:
- Allocate dedicated workqueue (llcp_wq) with WQ_UNBOUND to fix syzbot warning

 net/nfc/llcp.h      |   1 +
 net/nfc/llcp_core.c | 123 +++++++++++++++++++++++++++-----------------
 net/nfc/llcp_sock.c |  49 +++++++++++++-----
 3 files changed, 115 insertions(+), 58 deletions(-)

diff --git a/net/nfc/llcp.h b/net/nfc/llcp.h
index d8345ed57c95..23ae7a0112d3 100644
--- a/net/nfc/llcp.h
+++ b/net/nfc/llcp.h
@@ -91,6 +91,7 @@ struct nfc_llcp_local {
 	struct hlist_head pending_sdreqs;
 	struct timer_list sdreq_timer;
 	struct work_struct sdreq_timeout_work;
+	struct work_struct release_work;
 	u8 sdreq_next_tid;
 
 	/* sockets array */
diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index bd6361e2efa4..23553e7426ec 100644
--- a/net/nfc/llcp_core.c
+++ b/net/nfc/llcp_core.c
@@ -20,6 +20,8 @@ static LIST_HEAD(llcp_devices);
 /* Protects llcp_devices list */
 static DEFINE_SPINLOCK(llcp_devices_lock);
 
+static struct workqueue_struct *llcp_wq;
+
 static void nfc_llcp_rx_skb(struct nfc_llcp_local *local, struct sk_buff *skb);
 
 void nfc_llcp_sock_link(struct llcp_sock_list *l, struct sock *sk)
@@ -63,21 +65,33 @@ static void nfc_llcp_socket_purge(struct nfc_llcp_sock *sock)
 	}
 }
 
+static struct sock *nfc_llcp_sock_list_pop(struct llcp_sock_list *l)
+{
+	struct sock *sk;
+
+	write_lock(&l->lock);
+	sk = sk_head(&l->head);
+	if (sk) {
+		sock_hold(sk);
+		sk_del_node_init(sk);
+	}
+	write_unlock(&l->lock);
+
+	return sk;
+}
+
 static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
 				    int err)
 {
 	struct sock *sk;
-	struct hlist_node *tmp;
 	struct nfc_llcp_sock *llcp_sock;
 
 	skb_queue_purge(&local->tx_queue);
 
-	write_lock(&local->sockets.lock);
-
-	sk_for_each_safe(sk, tmp, &local->sockets.head) {
+	while ((sk = nfc_llcp_sock_list_pop(&local->sockets))) {
 		llcp_sock = nfc_llcp_sock(sk);
 
-		bh_lock_sock(sk);
+		lock_sock(sk);
 
 		nfc_llcp_socket_purge(llcp_sock);
 
@@ -91,17 +105,27 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
 			list_for_each_entry_safe(lsk, n,
 						 &llcp_sock->accept_queue,
 						 accept_queue) {
+				bool put_creation = false;
+
 				accept_sk = &lsk->sk;
-				bh_lock_sock(accept_sk);
+				lock_sock_nested(accept_sk,
+						 SINGLE_DEPTH_NESTING);
 
-				nfc_llcp_accept_unlink(accept_sk);
+				if (nfc_llcp_sock(accept_sk)->parent == sk) {
+					nfc_llcp_accept_unlink(accept_sk);
+					nfc_llcp_sock_unlink(&local->sockets, accept_sk);
 
-				if (err)
-					accept_sk->sk_err = err;
-				accept_sk->sk_state = LLCP_CLOSED;
-				accept_sk->sk_state_change(sk);
+					if (err)
+						accept_sk->sk_err = err;
+					accept_sk->sk_state = LLCP_CLOSED;
+					accept_sk->sk_state_change(accept_sk);
+					sock_orphan(accept_sk);
+					put_creation = true;
+				}
 
-				bh_unlock_sock(accept_sk);
+				release_sock(accept_sk);
+				if (put_creation)
+					sock_put(accept_sk); /* creation ref */
 			}
 		}
 
@@ -110,23 +134,18 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
 		sk->sk_state = LLCP_CLOSED;
 		sk->sk_state_change(sk);
 
-		bh_unlock_sock(sk);
-
-		sk_del_node_init(sk);
+		release_sock(sk);
+		sock_put(sk);
 	}
 
-	write_unlock(&local->sockets.lock);
-
 	/* If we still have a device, we keep the RAW sockets alive */
 	if (device == true)
 		return;
 
-	write_lock(&local->raw_sockets.lock);
-
-	sk_for_each_safe(sk, tmp, &local->raw_sockets.head) {
+	while ((sk = nfc_llcp_sock_list_pop(&local->raw_sockets))) {
 		llcp_sock = nfc_llcp_sock(sk);
 
-		bh_lock_sock(sk);
+		lock_sock(sk);
 
 		nfc_llcp_socket_purge(llcp_sock);
 
@@ -135,26 +154,20 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
 		sk->sk_state = LLCP_CLOSED;
 		sk->sk_state_change(sk);
 
-		bh_unlock_sock(sk);
-
-		sk_del_node_init(sk);
+		release_sock(sk);
+		sock_put(sk);
 	}
-
-	write_unlock(&local->raw_sockets.lock);
 }
 
 static struct nfc_llcp_local *nfc_llcp_local_get(struct nfc_llcp_local *local)
 {
-	/* Since using nfc_llcp_local may result in usage of nfc_dev, whenever
-	 * we hold a reference to local, we also need to hold a reference to
-	 * the device to avoid UAF.
-	 */
-	if (!nfc_get_device(local->dev->idx))
+	if (!local)
 		return NULL;
 
-	kref_get(&local->ref);
+	if (kref_get_unless_zero(&local->ref))
+		return local;
 
-	return local;
+	return NULL;
 }
 
 static void local_cleanup(struct nfc_llcp_local *local)
@@ -172,30 +185,34 @@ static void local_cleanup(struct nfc_llcp_local *local)
 	nfc_llcp_free_sdp_tlv_list(&local->pending_sdreqs);
 }
 
+static void local_release_work(struct work_struct *work)
+{
+	struct nfc_llcp_local *local;
+	struct nfc_dev *dev;
+
+	local = container_of(work, struct nfc_llcp_local, release_work);
+	dev = local->dev;
+
+	local_cleanup(local);
+	kfree(local);
+	nfc_put_device(dev);
+}
+
 static void local_release(struct kref *ref)
 {
 	struct nfc_llcp_local *local;
 
 	local = container_of(ref, struct nfc_llcp_local, ref);
 
-	local_cleanup(local);
-	kfree(local);
+	queue_work(llcp_wq, &local->release_work);
 }
 
 int nfc_llcp_local_put(struct nfc_llcp_local *local)
 {
-	struct nfc_dev *dev;
-	int ret;
-
-	if (local == NULL)
+	if (!local)
 		return 0;
 
-	dev = local->dev;
-
-	ret = kref_put(&local->ref, local_release);
-	nfc_put_device(dev);
-
-	return ret;
+	return kref_put(&local->ref, local_release);
 }
 
 static struct nfc_llcp_sock *nfc_llcp_sock_get(struct nfc_llcp_local *local,
@@ -1705,6 +1722,7 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
 	INIT_WORK(&local->rx_work, nfc_llcp_rx_work);
 
 	INIT_WORK(&local->timeout_work, nfc_llcp_timeout_work);
+	INIT_WORK(&local->release_work, local_release_work);
 
 	rwlock_init(&local->sockets.lock);
 	rwlock_init(&local->connecting_sockets.lock);
@@ -1748,10 +1766,23 @@ void nfc_llcp_unregister_device(struct nfc_dev *dev)
 
 int __init nfc_llcp_init(void)
 {
-	return nfc_llcp_sock_init();
+	int ret;
+
+	llcp_wq = alloc_workqueue("nfc_llcp_wq", WQ_UNBOUND, 0);
+	if (!llcp_wq)
+		return -ENOMEM;
+
+	ret = nfc_llcp_sock_init();
+	if (ret) {
+		destroy_workqueue(llcp_wq);
+		return ret;
+	}
+
+	return 0;
 }
 
 void nfc_llcp_exit(void)
 {
 	nfc_llcp_sock_exit();
+	destroy_workqueue(llcp_wq);
 }
diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
index 5558d8a4d48b..ce6875eb58fb 100644
--- a/net/nfc/llcp_sock.c
+++ b/net/nfc/llcp_sock.c
@@ -392,11 +392,12 @@ void nfc_llcp_accept_unlink(struct sock *sk)
 
 	pr_debug("state %d\n", sk->sk_state);
 
-	list_del_init(&llcp_sock->accept_queue);
-	sk_acceptq_removed(llcp_sock->parent);
-	llcp_sock->parent = NULL;
-
-	sock_put(sk);
+	if (llcp_sock->parent) {
+		list_del_init(&llcp_sock->accept_queue);
+		sk_acceptq_removed(llcp_sock->parent);
+		llcp_sock->parent = NULL;
+		sock_put(sk);
+	}
 }
 
 void nfc_llcp_accept_enqueue(struct sock *parent, struct sock *sk)
@@ -423,12 +424,20 @@ struct sock *nfc_llcp_accept_dequeue(struct sock *parent,
 
 	list_for_each_entry_safe(lsk, n, &llcp_parent->accept_queue,
 				 accept_queue) {
+		struct nfc_llcp_local *local;
+
 		sk = &lsk->sk;
-		lock_sock(sk);
+		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
 
 		if (sk->sk_state == LLCP_CLOSED) {
-			release_sock(sk);
+			local = nfc_llcp_sock(sk)->local;
+
 			nfc_llcp_accept_unlink(sk);
+			if (local)
+				nfc_llcp_sock_unlink(&local->sockets, sk);
+			sock_orphan(sk);
+			release_sock(sk);
+			sock_put(sk);
 			continue;
 		}
 
@@ -464,7 +473,7 @@ static int llcp_sock_accept(struct socket *sock, struct socket *newsock,
 
 	pr_debug("parent %p\n", sk);
 
-	lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+	lock_sock(sk);
 
 	if (sk->sk_state != LLCP_LISTEN) {
 		ret = -EBADFD;
@@ -490,7 +499,12 @@ static int llcp_sock_accept(struct socket *sock, struct socket *newsock,
 
 		release_sock(sk);
 		timeo = schedule_timeout(timeo);
-		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
+		lock_sock(sk);
+
+		if (sk->sk_state != LLCP_LISTEN) {
+			ret = -EBADFD;
+			break;
+		}
 	}
 	__set_current_state(TASK_RUNNING);
 	remove_wait_queue(sk_sleep(sk), &wait);
@@ -629,13 +643,24 @@ static int llcp_sock_release(struct socket *sock)
 
 		list_for_each_entry_safe(lsk, n, &llcp_sock->accept_queue,
 					 accept_queue) {
+			bool put_creation = false;
+
 			accept_sk = &lsk->sk;
-			lock_sock(accept_sk);
+			lock_sock_nested(accept_sk, SINGLE_DEPTH_NESTING);
 
-			nfc_llcp_send_disconnect(lsk);
-			nfc_llcp_accept_unlink(accept_sk);
+			if (nfc_llcp_sock(accept_sk)->parent == sk) {
+				nfc_llcp_send_disconnect(lsk);
+				nfc_llcp_accept_unlink(accept_sk);
+				nfc_llcp_sock_unlink(&local->sockets, accept_sk);
+
+				accept_sk->sk_state = LLCP_CLOSED;
+				sock_orphan(accept_sk);
+				put_creation = true;
+			}
 
 			release_sock(accept_sk);
+			if (put_creation)
+				sock_put(accept_sk); /* creation ref */
 		}
 	}
 
-- 
2.55.0.966.g6673acef38-goog

Re: [PATCH v3 1/1] nfc: llcp: Fix race condition in accept_queue lifecycle

[Lee Jones]

[INTENTIONAL TOP-POST]

Good morning David,

Just checking that this is on your radar this time [0].

Please let me know if you'd prefer a [RESEND].

[0] https://lore.kernel.org/all/016b7162-b633-4561-a003-526c9d9c5f21@ixit.cz/

Kind regards,
Lee

> In nfc_llcp_socket_release(), sockets and listener accept queues are
> walked under the local sockets rwlock and bh_lock_sock().  However,
> bh_lock_sock() does not synchronise against process-context lock_sock()
> held by nfc_llcp_accept_dequeue() during accept().  Because
> socket_release() does not check sock_owned_by_user(), both paths can
> concurrently unlink and release the same child socket, resulting in
> use-after-free or a NULL pointer dereference of child->parent in
> nfc_llcp_accept_unlink().
> 
> Fix this synchronisation race by having nfc_llcp_socket_release() use
> process-context lock_sock() instead of bh_lock_sock():
> 
> 1. Pop sockets from the local sockets list under the write lock using
>    nfc_llcp_sock_list_pop() so lock_sock() can be acquired without
>    holding the rwlock.
> 
> 2. Because lock_sock() can sleep, defer the final release of the
>    nfc_llcp_local structure to a workqueue (release_work).  This avoids
>    a sleeping-in-atomic bug when the last local reference is dropped
>    from softirq context.  Additionally, hold a single device reference
>    on local from registration until final destruction.
> 
> 3. In nfc_llcp_local_get(), use kref_get_unless_zero() to prevent
>    resurrecting a local object whose teardown has been scheduled.
> 
> 4. In llcp_sock_accept(), verify that the listener socket state is still
>    LLCP_LISTEN after waking from schedule_timeout() to prevent hangs if
>    the listener is closed concurrently.
> 
> 5. When unlinking unaccepted child sockets during listener release,
>    unlink them from local->sockets, call sock_orphan(), and drop their
>    initial sk_alloc creation reference via sock_put().
> 
> 6. Make nfc_llcp_accept_unlink() idempotent by guarding parent access with
>    a NULL check.
> 
> Fixes: 50b78b2a6500 ("NFC: Fix sleeping in atomic when releasing socket")
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
> 
> v1 -> v2:
> - Defer local release to dedicated workqueue (llcp_wq) to avoid sleeping in atomic
> - Drain and destroy llcp_wq on module unload to prevent module exit race
> - Drop initial sk_alloc creation ref on unaccepted child sockets across all teardown paths
> - Use kref_get_unless_zero() in local_get to prevent resurrecting dying local objects
> - Check listener socket state after waking in llcp_sock_accept() to avoid hangs
> - Standardise Parent (0) -> Child (1) lockdep subclass nesting
> 
> v2 -> v3:
> - Allocate dedicated workqueue (llcp_wq) with WQ_UNBOUND to fix syzbot warning
> 
>  net/nfc/llcp.h      |   1 +
>  net/nfc/llcp_core.c | 123 +++++++++++++++++++++++++++-----------------
>  net/nfc/llcp_sock.c |  49 +++++++++++++-----
>  3 files changed, 115 insertions(+), 58 deletions(-)
> 
> diff --git a/net/nfc/llcp.h b/net/nfc/llcp.h
> index d8345ed57c95..23ae7a0112d3 100644
> --- a/net/nfc/llcp.h
> +++ b/net/nfc/llcp.h
> @@ -91,6 +91,7 @@ struct nfc_llcp_local {
>  	struct hlist_head pending_sdreqs;
>  	struct timer_list sdreq_timer;
>  	struct work_struct sdreq_timeout_work;
> +	struct work_struct release_work;
>  	u8 sdreq_next_tid;
>  
>  	/* sockets array */
> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index bd6361e2efa4..23553e7426ec 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c
> @@ -20,6 +20,8 @@ static LIST_HEAD(llcp_devices);
>  /* Protects llcp_devices list */
>  static DEFINE_SPINLOCK(llcp_devices_lock);
>  
> +static struct workqueue_struct *llcp_wq;
> +
>  static void nfc_llcp_rx_skb(struct nfc_llcp_local *local, struct sk_buff *skb);
>  
>  void nfc_llcp_sock_link(struct llcp_sock_list *l, struct sock *sk)
> @@ -63,21 +65,33 @@ static void nfc_llcp_socket_purge(struct nfc_llcp_sock *sock)
>  	}
>  }
>  
> +static struct sock *nfc_llcp_sock_list_pop(struct llcp_sock_list *l)
> +{
> +	struct sock *sk;
> +
> +	write_lock(&l->lock);
> +	sk = sk_head(&l->head);
> +	if (sk) {
> +		sock_hold(sk);
> +		sk_del_node_init(sk);
> +	}
> +	write_unlock(&l->lock);
> +
> +	return sk;
> +}
> +
>  static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
>  				    int err)
>  {
>  	struct sock *sk;
> -	struct hlist_node *tmp;
>  	struct nfc_llcp_sock *llcp_sock;
>  
>  	skb_queue_purge(&local->tx_queue);
>  
> -	write_lock(&local->sockets.lock);
> -
> -	sk_for_each_safe(sk, tmp, &local->sockets.head) {
> +	while ((sk = nfc_llcp_sock_list_pop(&local->sockets))) {
>  		llcp_sock = nfc_llcp_sock(sk);
>  
> -		bh_lock_sock(sk);
> +		lock_sock(sk);
>  
>  		nfc_llcp_socket_purge(llcp_sock);
>  
> @@ -91,17 +105,27 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
>  			list_for_each_entry_safe(lsk, n,
>  						 &llcp_sock->accept_queue,
>  						 accept_queue) {
> +				bool put_creation = false;
> +
>  				accept_sk = &lsk->sk;
> -				bh_lock_sock(accept_sk);
> +				lock_sock_nested(accept_sk,
> +						 SINGLE_DEPTH_NESTING);
>  
> -				nfc_llcp_accept_unlink(accept_sk);
> +				if (nfc_llcp_sock(accept_sk)->parent == sk) {
> +					nfc_llcp_accept_unlink(accept_sk);
> +					nfc_llcp_sock_unlink(&local->sockets, accept_sk);
>  
> -				if (err)
> -					accept_sk->sk_err = err;
> -				accept_sk->sk_state = LLCP_CLOSED;
> -				accept_sk->sk_state_change(sk);
> +					if (err)
> +						accept_sk->sk_err = err;
> +					accept_sk->sk_state = LLCP_CLOSED;
> +					accept_sk->sk_state_change(accept_sk);
> +					sock_orphan(accept_sk);
> +					put_creation = true;
> +				}
>  
> -				bh_unlock_sock(accept_sk);
> +				release_sock(accept_sk);
> +				if (put_creation)
> +					sock_put(accept_sk); /* creation ref */
>  			}
>  		}
>  
> @@ -110,23 +134,18 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
>  		sk->sk_state = LLCP_CLOSED;
>  		sk->sk_state_change(sk);
>  
> -		bh_unlock_sock(sk);
> -
> -		sk_del_node_init(sk);
> +		release_sock(sk);
> +		sock_put(sk);
>  	}
>  
> -	write_unlock(&local->sockets.lock);
> -
>  	/* If we still have a device, we keep the RAW sockets alive */
>  	if (device == true)
>  		return;
>  
> -	write_lock(&local->raw_sockets.lock);
> -
> -	sk_for_each_safe(sk, tmp, &local->raw_sockets.head) {
> +	while ((sk = nfc_llcp_sock_list_pop(&local->raw_sockets))) {
>  		llcp_sock = nfc_llcp_sock(sk);
>  
> -		bh_lock_sock(sk);
> +		lock_sock(sk);
>  
>  		nfc_llcp_socket_purge(llcp_sock);
>  
> @@ -135,26 +154,20 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
>  		sk->sk_state = LLCP_CLOSED;
>  		sk->sk_state_change(sk);
>  
> -		bh_unlock_sock(sk);
> -
> -		sk_del_node_init(sk);
> +		release_sock(sk);
> +		sock_put(sk);
>  	}
> -
> -	write_unlock(&local->raw_sockets.lock);
>  }
>  
>  static struct nfc_llcp_local *nfc_llcp_local_get(struct nfc_llcp_local *local)
>  {
> -	/* Since using nfc_llcp_local may result in usage of nfc_dev, whenever
> -	 * we hold a reference to local, we also need to hold a reference to
> -	 * the device to avoid UAF.
> -	 */
> -	if (!nfc_get_device(local->dev->idx))
> +	if (!local)
>  		return NULL;
>  
> -	kref_get(&local->ref);
> +	if (kref_get_unless_zero(&local->ref))
> +		return local;
>  
> -	return local;
> +	return NULL;
>  }
>  
>  static void local_cleanup(struct nfc_llcp_local *local)
> @@ -172,30 +185,34 @@ static void local_cleanup(struct nfc_llcp_local *local)
>  	nfc_llcp_free_sdp_tlv_list(&local->pending_sdreqs);
>  }
>  
> +static void local_release_work(struct work_struct *work)
> +{
> +	struct nfc_llcp_local *local;
> +	struct nfc_dev *dev;
> +
> +	local = container_of(work, struct nfc_llcp_local, release_work);
> +	dev = local->dev;
> +
> +	local_cleanup(local);
> +	kfree(local);
> +	nfc_put_device(dev);
> +}
> +
>  static void local_release(struct kref *ref)
>  {
>  	struct nfc_llcp_local *local;
>  
>  	local = container_of(ref, struct nfc_llcp_local, ref);
>  
> -	local_cleanup(local);
> -	kfree(local);
> +	queue_work(llcp_wq, &local->release_work);
>  }
>  
>  int nfc_llcp_local_put(struct nfc_llcp_local *local)
>  {
> -	struct nfc_dev *dev;
> -	int ret;
> -
> -	if (local == NULL)
> +	if (!local)
>  		return 0;
>  
> -	dev = local->dev;
> -
> -	ret = kref_put(&local->ref, local_release);
> -	nfc_put_device(dev);
> -
> -	return ret;
> +	return kref_put(&local->ref, local_release);
>  }
>  
>  static struct nfc_llcp_sock *nfc_llcp_sock_get(struct nfc_llcp_local *local,
> @@ -1705,6 +1722,7 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
>  	INIT_WORK(&local->rx_work, nfc_llcp_rx_work);
>  
>  	INIT_WORK(&local->timeout_work, nfc_llcp_timeout_work);
> +	INIT_WORK(&local->release_work, local_release_work);
>  
>  	rwlock_init(&local->sockets.lock);
>  	rwlock_init(&local->connecting_sockets.lock);
> @@ -1748,10 +1766,23 @@ void nfc_llcp_unregister_device(struct nfc_dev *dev)
>  
>  int __init nfc_llcp_init(void)
>  {
> -	return nfc_llcp_sock_init();
> +	int ret;
> +
> +	llcp_wq = alloc_workqueue("nfc_llcp_wq", WQ_UNBOUND, 0);
> +	if (!llcp_wq)
> +		return -ENOMEM;
> +
> +	ret = nfc_llcp_sock_init();
> +	if (ret) {
> +		destroy_workqueue(llcp_wq);
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  void nfc_llcp_exit(void)
>  {
>  	nfc_llcp_sock_exit();
> +	destroy_workqueue(llcp_wq);
>  }
> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index 5558d8a4d48b..ce6875eb58fb 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
> @@ -392,11 +392,12 @@ void nfc_llcp_accept_unlink(struct sock *sk)
>  
>  	pr_debug("state %d\n", sk->sk_state);
>  
> -	list_del_init(&llcp_sock->accept_queue);
> -	sk_acceptq_removed(llcp_sock->parent);
> -	llcp_sock->parent = NULL;
> -
> -	sock_put(sk);
> +	if (llcp_sock->parent) {
> +		list_del_init(&llcp_sock->accept_queue);
> +		sk_acceptq_removed(llcp_sock->parent);
> +		llcp_sock->parent = NULL;
> +		sock_put(sk);
> +	}
>  }
>  
>  void nfc_llcp_accept_enqueue(struct sock *parent, struct sock *sk)
> @@ -423,12 +424,20 @@ struct sock *nfc_llcp_accept_dequeue(struct sock *parent,
>  
>  	list_for_each_entry_safe(lsk, n, &llcp_parent->accept_queue,
>  				 accept_queue) {
> +		struct nfc_llcp_local *local;
> +
>  		sk = &lsk->sk;
> -		lock_sock(sk);
> +		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>  
>  		if (sk->sk_state == LLCP_CLOSED) {
> -			release_sock(sk);
> +			local = nfc_llcp_sock(sk)->local;
> +
>  			nfc_llcp_accept_unlink(sk);
> +			if (local)
> +				nfc_llcp_sock_unlink(&local->sockets, sk);
> +			sock_orphan(sk);
> +			release_sock(sk);
> +			sock_put(sk);
>  			continue;
>  		}
>  
> @@ -464,7 +473,7 @@ static int llcp_sock_accept(struct socket *sock, struct socket *newsock,
>  
>  	pr_debug("parent %p\n", sk);
>  
> -	lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> +	lock_sock(sk);
>  
>  	if (sk->sk_state != LLCP_LISTEN) {
>  		ret = -EBADFD;
> @@ -490,7 +499,12 @@ static int llcp_sock_accept(struct socket *sock, struct socket *newsock,
>  
>  		release_sock(sk);
>  		timeo = schedule_timeout(timeo);
> -		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> +		lock_sock(sk);
> +
> +		if (sk->sk_state != LLCP_LISTEN) {
> +			ret = -EBADFD;
> +			break;
> +		}
>  	}
>  	__set_current_state(TASK_RUNNING);
>  	remove_wait_queue(sk_sleep(sk), &wait);
> @@ -629,13 +643,24 @@ static int llcp_sock_release(struct socket *sock)
>  
>  		list_for_each_entry_safe(lsk, n, &llcp_sock->accept_queue,
>  					 accept_queue) {
> +			bool put_creation = false;
> +
>  			accept_sk = &lsk->sk;
> -			lock_sock(accept_sk);
> +			lock_sock_nested(accept_sk, SINGLE_DEPTH_NESTING);
>  
> -			nfc_llcp_send_disconnect(lsk);
> -			nfc_llcp_accept_unlink(accept_sk);
> +			if (nfc_llcp_sock(accept_sk)->parent == sk) {
> +				nfc_llcp_send_disconnect(lsk);
> +				nfc_llcp_accept_unlink(accept_sk);
> +				nfc_llcp_sock_unlink(&local->sockets, accept_sk);
> +
> +				accept_sk->sk_state = LLCP_CLOSED;
> +				sock_orphan(accept_sk);
> +				put_creation = true;
> +			}
>  
>  			release_sock(accept_sk);
> +			if (put_creation)
> +				sock_put(accept_sk); /* creation ref */
>  		}
>  	}
>  
> -- 
> 2.55.0.966.g6673acef38-goog
> 

-- 
Lee Jones

Re: [PATCH v3 1/1] nfc: llcp: Fix race condition in accept_queue lifecycle

[David Heidelberg]

Hello Lee,

thank you. Yes, it's.

David

On 08/09/2026 10:06, Lee Jones wrote:
> [INTENTIONAL TOP-POST]
> 
> Good morning David,
> 
> Just checking that this is on your radar this time [0].
> 
> Please let me know if you'd prefer a [RESEND].
> 
> [0] https://lore.kernel.org/all/016b7162-b633-4561-a003-526c9d9c5f21@ixit.cz/
> 
> Kind regards,
> Lee
> 
>> In nfc_llcp_socket_release(), sockets and listener accept queues are
>> walked under the local sockets rwlock and bh_lock_sock().  However,
>> bh_lock_sock() does not synchronise against process-context lock_sock()
>> held by nfc_llcp_accept_dequeue() during accept().  Because
>> socket_release() does not check sock_owned_by_user(), both paths can
>> concurrently unlink and release the same child socket, resulting in
>> use-after-free or a NULL pointer dereference of child->parent in
>> nfc_llcp_accept_unlink().
>>
>> Fix this synchronisation race by having nfc_llcp_socket_release() use
>> process-context lock_sock() instead of bh_lock_sock():
>>
>> 1. Pop sockets from the local sockets list under the write lock using
>>     nfc_llcp_sock_list_pop() so lock_sock() can be acquired without
>>     holding the rwlock.
>>
>> 2. Because lock_sock() can sleep, defer the final release of the
>>     nfc_llcp_local structure to a workqueue (release_work).  This avoids
>>     a sleeping-in-atomic bug when the last local reference is dropped
>>     from softirq context.  Additionally, hold a single device reference
>>     on local from registration until final destruction.
>>
>> 3. In nfc_llcp_local_get(), use kref_get_unless_zero() to prevent
>>     resurrecting a local object whose teardown has been scheduled.
>>
>> 4. In llcp_sock_accept(), verify that the listener socket state is still
>>     LLCP_LISTEN after waking from schedule_timeout() to prevent hangs if
>>     the listener is closed concurrently.
>>
>> 5. When unlinking unaccepted child sockets during listener release,
>>     unlink them from local->sockets, call sock_orphan(), and drop their
>>     initial sk_alloc creation reference via sock_put().
>>
>> 6. Make nfc_llcp_accept_unlink() idempotent by guarding parent access with
>>     a NULL check.
>>
>> Fixes: 50b78b2a6500 ("NFC: Fix sleeping in atomic when releasing socket")
>> Signed-off-by: Lee Jones <lee@kernel.org>
>> ---
>>
>> v1 -> v2:
>> - Defer local release to dedicated workqueue (llcp_wq) to avoid sleeping in atomic
>> - Drain and destroy llcp_wq on module unload to prevent module exit race
>> - Drop initial sk_alloc creation ref on unaccepted child sockets across all teardown paths
>> - Use kref_get_unless_zero() in local_get to prevent resurrecting dying local objects
>> - Check listener socket state after waking in llcp_sock_accept() to avoid hangs
>> - Standardise Parent (0) -> Child (1) lockdep subclass nesting
>>
>> v2 -> v3:
>> - Allocate dedicated workqueue (llcp_wq) with WQ_UNBOUND to fix syzbot warning
>>
>>   net/nfc/llcp.h      |   1 +
>>   net/nfc/llcp_core.c | 123 +++++++++++++++++++++++++++-----------------
>>   net/nfc/llcp_sock.c |  49 +++++++++++++-----
>>   3 files changed, 115 insertions(+), 58 deletions(-)
>>
>> diff --git a/net/nfc/llcp.h b/net/nfc/llcp.h
>> index d8345ed57c95..23ae7a0112d3 100644
>> --- a/net/nfc/llcp.h
>> +++ b/net/nfc/llcp.h
>> @@ -91,6 +91,7 @@ struct nfc_llcp_local {
>>   	struct hlist_head pending_sdreqs;
>>   	struct timer_list sdreq_timer;
>>   	struct work_struct sdreq_timeout_work;
>> +	struct work_struct release_work;
>>   	u8 sdreq_next_tid;
>>   
>>   	/* sockets array */
>> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
>> index bd6361e2efa4..23553e7426ec 100644
>> --- a/net/nfc/llcp_core.c
>> +++ b/net/nfc/llcp_core.c
>> @@ -20,6 +20,8 @@ static LIST_HEAD(llcp_devices);
>>   /* Protects llcp_devices list */
>>   static DEFINE_SPINLOCK(llcp_devices_lock);
>>   
>> +static struct workqueue_struct *llcp_wq;
>> +
>>   static void nfc_llcp_rx_skb(struct nfc_llcp_local *local, struct sk_buff *skb);
>>   
>>   void nfc_llcp_sock_link(struct llcp_sock_list *l, struct sock *sk)
>> @@ -63,21 +65,33 @@ static void nfc_llcp_socket_purge(struct nfc_llcp_sock *sock)
>>   	}
>>   }
>>   
>> +static struct sock *nfc_llcp_sock_list_pop(struct llcp_sock_list *l)
>> +{
>> +	struct sock *sk;
>> +
>> +	write_lock(&l->lock);
>> +	sk = sk_head(&l->head);
>> +	if (sk) {
>> +		sock_hold(sk);
>> +		sk_del_node_init(sk);
>> +	}
>> +	write_unlock(&l->lock);
>> +
>> +	return sk;
>> +}
>> +
>>   static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
>>   				    int err)
>>   {
>>   	struct sock *sk;
>> -	struct hlist_node *tmp;
>>   	struct nfc_llcp_sock *llcp_sock;
>>   
>>   	skb_queue_purge(&local->tx_queue);
>>   
>> -	write_lock(&local->sockets.lock);
>> -
>> -	sk_for_each_safe(sk, tmp, &local->sockets.head) {
>> +	while ((sk = nfc_llcp_sock_list_pop(&local->sockets))) {
>>   		llcp_sock = nfc_llcp_sock(sk);
>>   
>> -		bh_lock_sock(sk);
>> +		lock_sock(sk);
>>   
>>   		nfc_llcp_socket_purge(llcp_sock);
>>   
>> @@ -91,17 +105,27 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
>>   			list_for_each_entry_safe(lsk, n,
>>   						 &llcp_sock->accept_queue,
>>   						 accept_queue) {
>> +				bool put_creation = false;
>> +
>>   				accept_sk = &lsk->sk;
>> -				bh_lock_sock(accept_sk);
>> +				lock_sock_nested(accept_sk,
>> +						 SINGLE_DEPTH_NESTING);
>>   
>> -				nfc_llcp_accept_unlink(accept_sk);
>> +				if (nfc_llcp_sock(accept_sk)->parent == sk) {
>> +					nfc_llcp_accept_unlink(accept_sk);
>> +					nfc_llcp_sock_unlink(&local->sockets, accept_sk);
>>   
>> -				if (err)
>> -					accept_sk->sk_err = err;
>> -				accept_sk->sk_state = LLCP_CLOSED;
>> -				accept_sk->sk_state_change(sk);
>> +					if (err)
>> +						accept_sk->sk_err = err;
>> +					accept_sk->sk_state = LLCP_CLOSED;
>> +					accept_sk->sk_state_change(accept_sk);
>> +					sock_orphan(accept_sk);
>> +					put_creation = true;
>> +				}
>>   
>> -				bh_unlock_sock(accept_sk);
>> +				release_sock(accept_sk);
>> +				if (put_creation)
>> +					sock_put(accept_sk); /* creation ref */
>>   			}
>>   		}
>>   
>> @@ -110,23 +134,18 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
>>   		sk->sk_state = LLCP_CLOSED;
>>   		sk->sk_state_change(sk);
>>   
>> -		bh_unlock_sock(sk);
>> -
>> -		sk_del_node_init(sk);
>> +		release_sock(sk);
>> +		sock_put(sk);
>>   	}
>>   
>> -	write_unlock(&local->sockets.lock);
>> -
>>   	/* If we still have a device, we keep the RAW sockets alive */
>>   	if (device == true)
>>   		return;
>>   
>> -	write_lock(&local->raw_sockets.lock);
>> -
>> -	sk_for_each_safe(sk, tmp, &local->raw_sockets.head) {
>> +	while ((sk = nfc_llcp_sock_list_pop(&local->raw_sockets))) {
>>   		llcp_sock = nfc_llcp_sock(sk);
>>   
>> -		bh_lock_sock(sk);
>> +		lock_sock(sk);
>>   
>>   		nfc_llcp_socket_purge(llcp_sock);
>>   
>> @@ -135,26 +154,20 @@ static void nfc_llcp_socket_release(struct nfc_llcp_local *local, bool device,
>>   		sk->sk_state = LLCP_CLOSED;
>>   		sk->sk_state_change(sk);
>>   
>> -		bh_unlock_sock(sk);
>> -
>> -		sk_del_node_init(sk);
>> +		release_sock(sk);
>> +		sock_put(sk);
>>   	}
>> -
>> -	write_unlock(&local->raw_sockets.lock);
>>   }
>>   
>>   static struct nfc_llcp_local *nfc_llcp_local_get(struct nfc_llcp_local *local)
>>   {
>> -	/* Since using nfc_llcp_local may result in usage of nfc_dev, whenever
>> -	 * we hold a reference to local, we also need to hold a reference to
>> -	 * the device to avoid UAF.
>> -	 */
>> -	if (!nfc_get_device(local->dev->idx))
>> +	if (!local)
>>   		return NULL;
>>   
>> -	kref_get(&local->ref);
>> +	if (kref_get_unless_zero(&local->ref))
>> +		return local;
>>   
>> -	return local;
>> +	return NULL;
>>   }
>>   
>>   static void local_cleanup(struct nfc_llcp_local *local)
>> @@ -172,30 +185,34 @@ static void local_cleanup(struct nfc_llcp_local *local)
>>   	nfc_llcp_free_sdp_tlv_list(&local->pending_sdreqs);
>>   }
>>   
>> +static void local_release_work(struct work_struct *work)
>> +{
>> +	struct nfc_llcp_local *local;
>> +	struct nfc_dev *dev;
>> +
>> +	local = container_of(work, struct nfc_llcp_local, release_work);
>> +	dev = local->dev;
>> +
>> +	local_cleanup(local);
>> +	kfree(local);
>> +	nfc_put_device(dev);
>> +}
>> +
>>   static void local_release(struct kref *ref)
>>   {
>>   	struct nfc_llcp_local *local;
>>   
>>   	local = container_of(ref, struct nfc_llcp_local, ref);
>>   
>> -	local_cleanup(local);
>> -	kfree(local);
>> +	queue_work(llcp_wq, &local->release_work);
>>   }
>>   
>>   int nfc_llcp_local_put(struct nfc_llcp_local *local)
>>   {
>> -	struct nfc_dev *dev;
>> -	int ret;
>> -
>> -	if (local == NULL)
>> +	if (!local)
>>   		return 0;
>>   
>> -	dev = local->dev;
>> -
>> -	ret = kref_put(&local->ref, local_release);
>> -	nfc_put_device(dev);
>> -
>> -	return ret;
>> +	return kref_put(&local->ref, local_release);
>>   }
>>   
>>   static struct nfc_llcp_sock *nfc_llcp_sock_get(struct nfc_llcp_local *local,
>> @@ -1705,6 +1722,7 @@ int nfc_llcp_register_device(struct nfc_dev *ndev)
>>   	INIT_WORK(&local->rx_work, nfc_llcp_rx_work);
>>   
>>   	INIT_WORK(&local->timeout_work, nfc_llcp_timeout_work);
>> +	INIT_WORK(&local->release_work, local_release_work);
>>   
>>   	rwlock_init(&local->sockets.lock);
>>   	rwlock_init(&local->connecting_sockets.lock);
>> @@ -1748,10 +1766,23 @@ void nfc_llcp_unregister_device(struct nfc_dev *dev)
>>   
>>   int __init nfc_llcp_init(void)
>>   {
>> -	return nfc_llcp_sock_init();
>> +	int ret;
>> +
>> +	llcp_wq = alloc_workqueue("nfc_llcp_wq", WQ_UNBOUND, 0);
>> +	if (!llcp_wq)
>> +		return -ENOMEM;
>> +
>> +	ret = nfc_llcp_sock_init();
>> +	if (ret) {
>> +		destroy_workqueue(llcp_wq);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   void nfc_llcp_exit(void)
>>   {
>>   	nfc_llcp_sock_exit();
>> +	destroy_workqueue(llcp_wq);
>>   }
>> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
>> index 5558d8a4d48b..ce6875eb58fb 100644
>> --- a/net/nfc/llcp_sock.c
>> +++ b/net/nfc/llcp_sock.c
>> @@ -392,11 +392,12 @@ void nfc_llcp_accept_unlink(struct sock *sk)
>>   
>>   	pr_debug("state %d\n", sk->sk_state);
>>   
>> -	list_del_init(&llcp_sock->accept_queue);
>> -	sk_acceptq_removed(llcp_sock->parent);
>> -	llcp_sock->parent = NULL;
>> -
>> -	sock_put(sk);
>> +	if (llcp_sock->parent) {
>> +		list_del_init(&llcp_sock->accept_queue);
>> +		sk_acceptq_removed(llcp_sock->parent);
>> +		llcp_sock->parent = NULL;
>> +		sock_put(sk);
>> +	}
>>   }
>>   
>>   void nfc_llcp_accept_enqueue(struct sock *parent, struct sock *sk)
>> @@ -423,12 +424,20 @@ struct sock *nfc_llcp_accept_dequeue(struct sock *parent,
>>   
>>   	list_for_each_entry_safe(lsk, n, &llcp_parent->accept_queue,
>>   				 accept_queue) {
>> +		struct nfc_llcp_local *local;
>> +
>>   		sk = &lsk->sk;
>> -		lock_sock(sk);
>> +		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>>   
>>   		if (sk->sk_state == LLCP_CLOSED) {
>> -			release_sock(sk);
>> +			local = nfc_llcp_sock(sk)->local;
>> +
>>   			nfc_llcp_accept_unlink(sk);
>> +			if (local)
>> +				nfc_llcp_sock_unlink(&local->sockets, sk);
>> +			sock_orphan(sk);
>> +			release_sock(sk);
>> +			sock_put(sk);
>>   			continue;
>>   		}
>>   
>> @@ -464,7 +473,7 @@ static int llcp_sock_accept(struct socket *sock, struct socket *newsock,
>>   
>>   	pr_debug("parent %p\n", sk);
>>   
>> -	lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>> +	lock_sock(sk);
>>   
>>   	if (sk->sk_state != LLCP_LISTEN) {
>>   		ret = -EBADFD;
>> @@ -490,7 +499,12 @@ static int llcp_sock_accept(struct socket *sock, struct socket *newsock,
>>   
>>   		release_sock(sk);
>>   		timeo = schedule_timeout(timeo);
>> -		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>> +		lock_sock(sk);
>> +
>> +		if (sk->sk_state != LLCP_LISTEN) {
>> +			ret = -EBADFD;
>> +			break;
>> +		}
>>   	}
>>   	__set_current_state(TASK_RUNNING);
>>   	remove_wait_queue(sk_sleep(sk), &wait);
>> @@ -629,13 +643,24 @@ static int llcp_sock_release(struct socket *sock)
>>   
>>   		list_for_each_entry_safe(lsk, n, &llcp_sock->accept_queue,
>>   					 accept_queue) {
>> +			bool put_creation = false;
>> +
>>   			accept_sk = &lsk->sk;
>> -			lock_sock(accept_sk);
>> +			lock_sock_nested(accept_sk, SINGLE_DEPTH_NESTING);
>>   
>> -			nfc_llcp_send_disconnect(lsk);
>> -			nfc_llcp_accept_unlink(accept_sk);
>> +			if (nfc_llcp_sock(accept_sk)->parent == sk) {
>> +				nfc_llcp_send_disconnect(lsk);
>> +				nfc_llcp_accept_unlink(accept_sk);
>> +				nfc_llcp_sock_unlink(&local->sockets, accept_sk);
>> +
>> +				accept_sk->sk_state = LLCP_CLOSED;
>> +				sock_orphan(accept_sk);
>> +				put_creation = true;
>> +			}
>>   
>>   			release_sock(accept_sk);
>> +			if (put_creation)
>> +				sock_put(accept_sk); /* creation ref */
>>   		}
>>   	}
>>   
>> -- 
>> 2.55.0.966.g6673acef38-goog
>>
>