[PATCH net RESEND] nfc: llcp: Fix list corruption / refcount desync in nfc_llcp_recv_dm()

[PATCH net RESEND] nfc: llcp: Fix list corruption / refcount desync in nfc_llcp_recv_dm()

[Aldo Ariel Panzardo]

nfc_llcp_recv_dm() handles DM(NOBOUND)/DM(REJ) for a socket that is still
linked on local->connecting_sockets: it looks the socket up with
nfc_llcp_connecting_sock_get(), sets sk->sk_state = LLCP_CLOSED and
returns, without taking the socket lock and without unlinking the socket
from the connecting_sockets list.

llcp_sock_release() selects the list to unlink from by sk_state: a socket
in LLCP_CONNECTING is unlinked from connecting_sockets, otherwise from the
sockets list.  Because recv_dm left the socket physically on
connecting_sockets but in the LLCP_CLOSED state, release() takes the else
branch and calls nfc_llcp_sock_unlink(&local->sockets, sk).  That runs
sk_del_node_init() while holding sockets.lock, i.e. it removes the socket
from the connecting_sockets hlist under the wrong lock.  A concurrent
connect() linking another socket onto connecting_sockets under
connecting_sockets.lock then mutates the same hlist unserialized, which
corrupts the list and desyncs the sk_add_node()/sk_del_node_init()
sock_hold()/__sock_put() pairing.  An unprivileged local process holding
LLCP sockets, with the DM supplied by the remote peer over an established
LLCP link, can drive this to leak kernel sockets without bound (the
mis-decrement goes through the non-freeing __sock_put() path, so the
object is never released), leading to memory exhaustion / DoS.

This is the same class of bug that was fixed in the sibling handler
nfc_llcp_recv_cc() by commit b493ea2765cc ("nfc: llcp: Fix use-after-free
race in nfc_llcp_recv_cc()"); recv_dm did not receive the equivalent fix.

Fix it the same way: take lock_sock(), re-check that the socket is still
hashed (release() may have won the race), and for the NOBOUND/REJ case
unlink it from connecting_sockets before moving it to LLCP_CLOSED.  The
unlink drops the connecting_sockets membership reference via
sk_del_node_init(), leaving the socket unhashed, so the later
nfc_llcp_sock_unlink() in llcp_sock_release() becomes a no-op and no
double put occurs.

Fixes: a69f32af86e3 ("NFC: Socket linked list")
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 net/nfc/llcp_core.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index cac1b5487..bd6361e2e 100644
--- a/net/nfc/llcp_core.c
+++ b/net/nfc/llcp_core.c
@@ -1251,6 +1251,7 @@ static void nfc_llcp_recv_dm(struct nfc_llcp_local *local,
 	struct nfc_llcp_sock *llcp_sock;
 	struct sock *sk;
 	u8 dsap, ssap, reason;
+	bool connecting = false;
 
 	dsap = nfc_llcp_dsap(skb);
 	ssap = nfc_llcp_ssap(skb);
@@ -1262,6 +1263,7 @@ static void nfc_llcp_recv_dm(struct nfc_llcp_local *local,
 	case LLCP_DM_NOBOUND:
 	case LLCP_DM_REJ:
 		llcp_sock = nfc_llcp_connecting_sock_get(local, dsap);
+		connecting = true;
 		break;
 
 	default:
@@ -1276,10 +1278,33 @@ static void nfc_llcp_recv_dm(struct nfc_llcp_local *local,
 
 	sk = &llcp_sock->sk;
 
+	lock_sock(sk);
+
+	/* Check if socket was destroyed whilst waiting for the lock */
+	if (!sk_hashed(sk)) {
+		release_sock(sk);
+		nfc_llcp_sock_put(llcp_sock);
+		return;
+	}
+
+	/*
+	 * For DM(NOBOUND)/DM(REJ) the socket is still linked on the
+	 * connecting_sockets list.  Unlink it here, under the socket lock,
+	 * before moving it to LLCP_CLOSED: llcp_sock_release() selects the
+	 * list to unlink from by sk_state, so leaving a connecting socket
+	 * in the CLOSED state would make it unlink from the wrong list and
+	 * corrupt the connecting_sockets list / desync the socket refcount.
+	 * This mirrors nfc_llcp_recv_cc().
+	 */
+	if (connecting)
+		nfc_llcp_sock_unlink(&local->connecting_sockets, sk);
+
 	sk->sk_err = ENXIO;
 	sk->sk_state = LLCP_CLOSED;
 	sk->sk_state_change(sk);
 
+	release_sock(sk);
+
 	nfc_llcp_sock_put(llcp_sock);
 }
 
-- 
2.43.0

Re: [PATCH net RESEND] nfc: llcp: Fix list corruption / refcount desync in nfc_llcp_recv_dm()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David Heidelberg <david@ixit.cz>:

On Wed, 23 Sep 2026 10:33:39 -0300 you wrote:
> nfc_llcp_recv_dm() handles DM(NOBOUND)/DM(REJ) for a socket that is still
> linked on local->connecting_sockets: it looks the socket up with
> nfc_llcp_connecting_sock_get(), sets sk->sk_state = LLCP_CLOSED and
> returns, without taking the socket lock and without unlinking the socket
> from the connecting_sockets list.
> 
> llcp_sock_release() selects the list to unlink from by sk_state: a socket
> in LLCP_CONNECTING is unlinked from connecting_sockets, otherwise from the
> sockets list.  Because recv_dm left the socket physically on
> connecting_sockets but in the LLCP_CLOSED state, release() takes the else
> branch and calls nfc_llcp_sock_unlink(&local->sockets, sk).  That runs
> sk_del_node_init() while holding sockets.lock, i.e. it removes the socket
> from the connecting_sockets hlist under the wrong lock.  A concurrent
> connect() linking another socket onto connecting_sockets under
> connecting_sockets.lock then mutates the same hlist unserialized, which
> corrupts the list and desyncs the sk_add_node()/sk_del_node_init()
> sock_hold()/__sock_put() pairing.  An unprivileged local process holding
> LLCP sockets, with the DM supplied by the remote peer over an established
> LLCP link, can drive this to leak kernel sockets without bound (the
> mis-decrement goes through the non-freeing __sock_put() path, so the
> object is never released), leading to memory exhaustion / DoS.
> 
> [...]

Here is the summary with links:
  - [net,RESEND] nfc: llcp: Fix list corruption / refcount desync in nfc_llcp_recv_dm()
    https://git.kernel.org/netdev/net/c/bf1460acdf8c

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net RESEND] nfc: llcp: Fix list corruption / refcount desync in nfc_llcp_recv_dm()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] After lock_sock(), the patched nfc_llcp_recv_dm() only checks
  sk_hashed(sk).

Pre-existing issues:
- [High] After nfc_llcp_recv_dm() moves a CONNECTING socket to
  LLCP_CLOSED, the socket still holds everything connect() gave it:…
- [Medium] nfc_llcp_recv_dm() reads the DM reason from skb->data[2]
  (llcp_core.c:1258) without checking skb->len >= 3.

--

nfc: llcp: Fix list corruption / refcount desync in nfc_llcp_recv_dm()

With this patch, nfc_llcp_recv_dm() takes the socket lock and re-checks
that the socket is still hashed. For DM(NOBOUND) and DM(REJ) it also
unlinks the socket from local->connecting_sockets before moving it to
LLCP_CLOSED. This follows the earlier nfc_llcp_recv_cc() fix, so
llcp_sock_release() no longer unlinks a connecting socket from the wrong
list.

> diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
> index cac1b5487064d..bd6361e2efa4f 100644
> --- a/net/nfc/llcp_core.c
> +++ b/net/nfc/llcp_core.c

[ ... ]

> @@ -1262,6 +1263,7 @@ static void nfc_llcp_recv_dm(struct nfc_llcp_local *local,
>  	case LLCP_DM_NOBOUND:
>  	case LLCP_DM_REJ:
>  		llcp_sock = nfc_llcp_connecting_sock_get(local, dsap);
> +		connecting = true;
>  		break;

[Severity: Medium]
This is a pre-existing issue, but the reason byte that picks this branch
is read without a length check:

net/nfc/llcp_core.c:nfc_llcp_recv_dm() {
    ...
	reason = skb->data[2];
    ...
}

The only length check on the way in is in __nfc_llcp_recv():

	if (!pskb_may_pull(skb, LLCP_HEADER_SIZE)) {

LLCP_HEADER_SIZE is 2.

nfc_llcp_recv_agf() also accepts 2-byte inner PDUs:

	if (pdu_len < LLCP_HEADER_SIZE || pdu_len > skb->len) {

It then copies them into an nfc_alloc_recv_skb(pdu_len) buffer. For a
2-byte inner DM, data[2] is uninitialized tailroom. On the direct path it
is one byte past skb->len.

After this patch, that byte also decides whether the new
connecting_sockets unlink runs. Should there be a check such as
"if (skb->len < LLCP_HEADER_SIZE + 1) return;" before the reason is read?

[ ... ]

> @@ -1276,10 +1278,33 @@ static void nfc_llcp_recv_dm(struct nfc_llcp_local *local,
>  
>  	sk = &llcp_sock->sk;
>  
> +	lock_sock(sk);
> +
> +	/* Check if socket was destroyed whilst waiting for the lock */
> +	if (!sk_hashed(sk)) {
> +		release_sock(sk);
> +		nfc_llcp_sock_put(llcp_sock);
> +		return;
> +	}
> +
> +	/*
> +	 * For DM(NOBOUND)/DM(REJ) the socket is still linked on the
> +	 * connecting_sockets list.  Unlink it here, under the socket lock,
> +	 * before moving it to LLCP_CLOSED: llcp_sock_release() selects the
> +	 * list to unlink from by sk_state, so leaving a connecting socket
> +	 * in the CLOSED state would make it unlink from the wrong list and
> +	 * corrupt the connecting_sockets list / desync the socket refcount.
> +	 * This mirrors nfc_llcp_recv_cc().
> +	 */
> +	if (connecting)
> +		nfc_llcp_sock_unlink(&local->connecting_sockets, sk);

[Severity: High]
Is sk_hashed() enough to show the socket is still on
local->connecting_sockets at this point? The connecting flag is set
before lock_sock(). While this handler waits for the lock, the socket
could move to local->sockets.

Consider this sequence:

llcp_sock_connect()
    nfc_llcp_sock_link(&local->connecting_sockets, sk);
    /* sk_state is still LLCP_CLOSED */
                                  rx_work: nfc_llcp_recv_dm()
                                    nfc_llcp_connecting_sock_get()
                                      /* matches on ssap only */
                                      sock_hold(&llcp_sock->sk);
                                    lock_sock(sk);  /* waits */
    nfc_llcp_send_connect() fails
    sock_unlink:
      nfc_llcp_sock_unlink(&local->connecting_sockets, sk);
    release_sock(sk);

llcp_sock_bind() in another thread sharing the fd wins the lock
    sk->sk_state == LLCP_CLOSED, check passes
    nfc_llcp_sock_link(&local->sockets, sk);
    sk->sk_state = LLCP_BOUND;
                                    /* now gets the lock */
                                    sk_hashed(sk) is true
                                    nfc_llcp_sock_unlink(
                                      &local->connecting_sockets, sk);

A user can force the nfc_llcp_send_connect() failure. If sk_err is still
pending from an earlier DM, this call chain returns that error:

llcp_allocate_pdu()->nfc_alloc_send_skb()->sock_alloc_send_pskb()

At the end of this sequence, the socket is removed from the
local->sockets hlist while connecting_sockets.lock is held, not
sockets.lock. If bind used a different dev_idx, that lock may belong to a
different local. The newly bound socket is also closed with ENXIO.

Can this race with other writers of local->sockets, such as bind or close
of another socket, and corrupt that list? That is the same kind of
wrong-lock corruption the commit message describes.

Before this patch, nfc_llcp_recv_dm() never unlinked, so this path is
new. nfc_llcp_recv_cc() has the same hashed-only check.

For the connecting case, would it work to also require
sk->sk_state == LLCP_CONNECTING and llcp_sock->local == local after
lock_sock(), and bail out otherwise?

> +
>  	sk->sk_err = ENXIO;
>  	sk->sk_state = LLCP_CLOSED;
>  	sk->sk_state_change(sk);

[Severity: High]
This isn't a bug introduced by this patch, but after the socket is moved
to LLCP_CLOSED here, it still holds everything llcp_sock_connect() gave
it:

  - llcp_sock->local, with its kref and nfc_dev ref
  - the nfc_get_device() ref taken by connect
  - the SSAP bit from nfc_llcp_get_local_ssap()
  - the kmemdup'd service_name

llcp_sock_connect() rejects only two states:

	if (sk->sk_state == LLCP_CONNECTED) {
		ret = -EISCONN;
		goto error;
	}
	if (sk->sk_state == LLCP_CONNECTING) {

So an unprivileged user can call connect() again on the rejected socket.
The old values are then overwritten without being released:

	llcp_sock->dev = dev;
	llcp_sock->local = local;
	llcp_sock->ssap = nfc_llcp_get_local_ssap(local);
	...
	llcp_sock->service_name = kmemdup(addr->service_name,

Does each retry leak the old nfc_llcp_local kref, both nfc_dev refs, the
local SAP bit and the old service_name?

llcp_sock_destruct() drops the connect-time nfc_dev ref only for
connected sockets:

	if (sk->sk_state == LLCP_CONNECTED)
		nfc_put_device(llcp_sock->dev);

Each device has only LLCP_LOCAL_NUM_SAP (32) local SAPs. The leaked bits
are never cleared: nfc_llcp_mac_is_down() does not reset
local->local_sap, and the leaked local kref keeps local_release() from
running.

The trigger is a repeated non-blocking connect() to a peer that answers
DM(NOBOUND) or DM(REJ). That is the normal reply for an unknown service,
so no malicious peer is needed. After enough retries, every LLCP client
connect() on that device would fail with -ENOMEM until the device is
re-registered.

Before this patch, the same reconnect also added a node that was already
hashed. The patch removes that part, but the leaks remain.

There are two possible fixes. nfc_llcp_recv_dm() could do the same
teardown as the connect() error path (sock_unlink, sock_llcp_release,
sock_llcp_nullify). Or connect() could reject sockets whose
llcp_sock->local is already set.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923133339.2518641-1-qwe.aldo%40gmail.com

[PATCH net] nfc: llcp: check socket state after locking in nfc_llcp_recv_dm()

[Aldo Ariel Panzardo]

Between nfc_llcp_connecting_sock_get() and lock_sock(), another
thread can move the socket from connecting_sockets to sockets via
a failed connect + re-bind. After lock_sock(), the socket may no
longer be in LLCP_CONNECTING state, so unconditionally unlinking
from connecting_sockets corrupts the wrong list.

Add a state check after acquiring the socket lock: only unlink
from connecting_sockets if the socket is still LLCP_CONNECTING.

Fixes: bf1460acdf8c ("nfc: llcp: Fix list corruption / refcount desync in nfc_llcp_recv_dm()")
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 net/nfc/llcp_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/nfc/llcp_core.c b/net/nfc/llcp_core.c
index 74bf81700..acf113311 100644
--- a/net/nfc/llcp_core.c
+++ b/net/nfc/llcp_core.c
@@ -1326,7 +1326,7 @@ static void nfc_llcp_recv_dm(struct nfc_llcp_local *local,
 	 * corrupt the connecting_sockets list / desync the socket refcount.
 	 * This mirrors nfc_llcp_recv_cc().
 	 */
-	if (connecting)
+	if (connecting && sk->sk_state == LLCP_CONNECTING)
 		nfc_llcp_sock_unlink(&local->connecting_sockets, sk);
 
 	sk->sk_err = ENXIO;
-- 
2.43.0

Re: [PATCH net] nfc: llcp: check socket state after locking in nfc_llcp_recv_dm()

[David Heidelberg]

On 25/09/2026 04:35, Aldo Ariel Panzardo wrote:
> Between nfc_llcp_connecting_sock_get() and lock_sock(), another
> thread can move the socket from connecting_sockets to sockets via
> a failed connect + re-bind. After lock_sock(), the socket may no
> longer be in LLCP_CONNECTING state, so unconditionally unlinking
> from connecting_sockets corrupts the wrong list.
> 
> Add a state check after acquiring the socket lock: only unlink
> from connecting_sockets if the socket is still LLCP_CONNECTING.
> 
> Fixes: bf1460acdf8c ("nfc: llcp: Fix list corruption / refcount desync in nfc_llcp_recv_dm()")
> Cc: stable@vger.kernel.org
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
Thank you for your patch, but please never send new version OR another patches 
as follow up to existing thread.

Please read: https://docs.kernel.org/process/submitting-patches.html

Thank you
David