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

[netdev-bot+sashiko@kernel.org]

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