[PATCH] nfc: llcp: fix socket list self-loop and soft lockup on bound connect

[PATCH] nfc: llcp: fix socket list self-loop and soft lockup on bound connect

[Henry Martin]

llcp_sock_connect() rejects LLCP_CONNECTED and LLCP_CONNECTING but
not LLCP_BOUND, so a bind() followed by connect() links the same
sk->sk_node into both local->sockets and local->connecting_sockets.
When the peer answers with CC, nfc_llcp_recv_cc() removes the node
from the connecting list and re-adds it to the sockets list, where the
stale bind-time linkage turns the node into a self-loop; every later
sk_for_each() over local->sockets then spins forever (soft lockup) and
the socket refcount leaks.

Reject connect() in LLCP_BOUND state like the CONNECTED/CONNECTING
cases (a bound socket must be closed or unbound before connecting to
another service).  Failing fast is also the only honest option: by the
time the error path could restore the binding, connect() had already
overwritten the bound service name and the unwind destroys the bound
session regardless.

This vulnerability was discovered by Tencent CodeBuddy Security.

Cc: stable@vger.kernel.org
Fixes: a69f32af86e3 ("NFC: Socket linked list")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 net/nfc/llcp_sock.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
index 1e5ee4bcde684..33afd85f931a1 100644
--- a/net/nfc/llcp_sock.c
+++ b/net/nfc/llcp_sock.c
@@ -714,6 +714,15 @@
 		ret = -EINPROGRESS;
 		goto error;
 	}
+	/* A bound socket is already linked into local->sockets; letting
+	 * it connect() would link the same sk->sk_node into
+	 * local->connecting_sockets too, and the CC handler's re-add turns
+	 * it into a self-loop.  Reject like CONNECTED/CONNECTING.
+	 */
+	if (sk->sk_state == LLCP_BOUND) {
+		ret = -EISCONN;
+		goto error;
+	}

 	dev = nfc_get_device(addr->dev_idx);
 	if (dev == NULL) {
--
2.43.7

Re: [PATCH] nfc: llcp: fix socket list self-loop and soft lockup on bound connect

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] nfc: llcp: fix socket list self-loop and soft lockup on bound connect

[netdev-bot+sashiko]

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

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

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] The fix is incomplete.
- [Low] The commit message says a bound socket "must be closed or unbound
  before connecting to another service".

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a
  locking violation that a remote peer can trigger.

--

nfc: llcp: fix socket list self-loop and soft lockup on bound connect

This commit makes llcp_sock_connect() reject sockets in the LLCP_BOUND
state with -EISCONN. Without the check, a bound socket's sk_node gets
linked into both local->sockets and local->connecting_sockets. When the
CC handler then re-adds the node to local->sockets, it turns into a
self-loop.

> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index 1e5ee4bcde684..b1b5b6d319466 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
> @@ -714,6 +714,15 @@ static int llcp_sock_connect(struct socket *sock, struct sockaddr_unsized *_addr
>  		ret = -EINPROGRESS;
>  		goto error;
>  	}
> +	/* A bound socket is already linked into local->sockets; letting
> +	 * it connect() would link the same sk->sk_node into
> +	 * local->connecting_sockets too, and the CC handler's re-add turns
> +	 * it into a self-loop.  Reject like CONNECTED/CONNECTING.
> +	 */
> +	if (sk->sk_state == LLCP_BOUND) {

[Severity: High]
Is checking only LLCP_BOUND enough here? The comment above says the real
problem is that sk->sk_node is already linked into local->sockets. That
is also true in some other states.

llcp_sock_listen() moves a bound socket to LLCP_LISTEN without unlinking
it:

net/nfc/llcp_sock.c:llcp_sock_listen() {
    ...
	if ((sock->type != SOCK_SEQPACKET && sock->type != SOCK_STREAM) ||
	    sk->sk_state != LLCP_BOUND) {
    ...
	sk->sk_state = LLCP_LISTEN;
    ...
}

The sequence socket(AF_NFC, SOCK_STREAM, NFC_SOCKPROTO_LLCP), bind(),
listen(), connect() still gets past all three state checks in
llcp_sock_connect(). It then reaches this call while the node is still
hashed on local->sockets:

	nfc_llcp_sock_link(&local->connecting_sockets, sk);

When CC arrives, nfc_llcp_recv_cc() does:

	nfc_llcp_sock_unlink(&local->connecting_sockets, sk);
	nfc_llcp_sock_link(&local->sockets, sk);

Wouldn't that create the same self-loop, and the same soft lockup in
nfc_llcp_sock_get() and the other sk_for_each() walks of local->sockets?
Only SOCK_RAW requires CAP_NET_RAW, so an unprivileged user seems able to
reach this once a DEP link is up.

LLCP_CLOSED looks like it has the same problem. Connected sockets go onto
local->sockets through nfc_llcp_recv_cc(), or through
nfc_llcp_recv_connect() for accepted children. Later, nfc_llcp_recv_disc()
and nfc_llcp_recv_dm() (for reasons other than NOBOUND/REJ) set the state
to closed without unlinking:

net/nfc/llcp_core.c:nfc_llcp_recv_disc() {
    ...
	if (sk->sk_state == LLCP_CONNECTED) {
		nfc_put_device(local->dev);
		sk->sk_state = LLCP_CLOSED;
		sk->sk_state_change(sk);
	}
    ...
}

The remote peer decides when this happens. A later connect() on the same
fd would pass the new check and link the node a second time.

llcp_sock_bind() has the same gap. It only checks sk->sk_state !=
LLCP_CLOSED before it calls nfc_llcp_sock_link(&local->sockets, sk), and
the node may still be hashed at that point.

On both of these paths, connect() also overwrites llcp_sock->local,
llcp_sock->ssap, llcp_sock->reserved_ssap and llcp_sock->service_name.
Does that leak the earlier nfc_llcp_local reference, the earlier SAP
reservation and the earlier service_name buffer?

If connect() then fails, the sock_unlink label removes the node only from
local->connecting_sockets. Wouldn't local->sockets still point at it?

Would it be more robust to check list membership instead of adding one
more state? For example, connect() could fail unless sk->sk_state ==
LLCP_CLOSED && !sk_hashed(sk) && !llcp_sock->local. Another option is to
have the transitions to LLCP_CLOSED unlink the socket.

> +		ret = -EISCONN;

[Severity: Low]
The commit message says:

  (a bound socket must be closed or unbound before connecting to
  another service)

Is there a way to unbind an LLCP socket? llcp_sock_bind() requires
LLCP_CLOSED. llcp_sock_connect() requires sa_family == AF_NFC, so there
is no AF_UNSPEC disconnect. No user operation moves a socket from
LLCP_BOUND back to LLCP_CLOSED, so closing it seems to be the only
option.

Also, -EISCONN means "Transport endpoint is already connected", but this
socket is only bound. Elsewhere in this file, wrong-state checks such as
those in llcp_sock_bind() and llcp_sock_listen() return -EBADFD.

Could a non-blocking connect loop that treats EISCONN as success mistake
a bound-only socket for a connected one? Would -EBADFD be more
consistent here?

> +		goto error;
> +	}
>  
>  	dev = nfc_get_device(addr->dev_idx);
>  	if (dev == NULL) {

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. The
sock_unlink error path further down in llcp_sock_connect() seems to
assume sk is still on local->connecting_sockets:

sock_unlink:
	nfc_llcp_sock_unlink(&local->connecting_sockets, sk);

sock_wait_state() drops the socket lock while it sleeps:

		release_sock(sk);
		timeo = schedule_timeout(timeo);
		lock_sock(sk);
		set_current_state(TASK_INTERRUPTIBLE);

		err = sock_error(sk);

That gives the rx work a window to move sk between lists. Suppose a peer
sends an AGF frame containing a CC followed by a DM whose reason is not
NOBOUND or REJ. nfc_llcp_recv_agf() handles the two PDUs back to back:

nfc_llcp_recv_cc()
    nfc_llcp_sock_unlink(&local->connecting_sockets, sk);
    nfc_llcp_sock_link(&local->sockets, sk);
    sk->sk_state = LLCP_CONNECTED;

nfc_llcp_recv_dm()
    llcp_sock = nfc_llcp_sock_get(local, dsap, ssap);
    ...
    sk->sk_err = ENXIO;
    sk->sk_state = LLCP_CLOSED;

When the waiter gets the lock back, sock_error() returns -ENXIO and it
jumps to sock_unlink. sock_unlink then calls sk_del_node_init() on a node
that is now on local->sockets, while holding only connecting_sockets.lock.
A DISC combined with a pending signal reaches the same label.

Can this race with code that changes local->sockets under
local->sockets.lock? Examples are bind() or release() on other sockets,
nfc_llcp_accept_dequeue(), and nfc_llcp_recv_connect() handling a later
CONNECT PDU in the same AGF.

If so, a socket could be dropped from the list, or a socket that is about
to be freed could stay linked, which would be a use-after-free.

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