[PATCH net] nfc: llcp: Fix raw socket local ref leak on rebind

[PATCH net] nfc: llcp: Fix raw socket local ref leak on rebind

[Shuangpeng Bai]

Raw LLCP sockets own the reference returned by nfc_llcp_find_local().
When the bound NFC device is unregistered, nfc_llcp_socket_release()
sets raw sockets back to LLCP_CLOSED. It also unlinks them from
local->raw_sockets, but leaves llcp_sock->local pointing at that local.

A subsequent successful bind on the same socket gets a new local reference
and overwrites llcp_sock->local. The old local reference is then lost, and
the final socket release only drops the new local.

Drop any stale local reference after the new target local has been found,
but before overwriting llcp_sock->local. This keeps failed bind attempts
from changing the old reference while preventing a successful rebind from
leaking it.

Fixes: e6a3a4bb856a ("NFC: llcp: Clean raw sockets from nfc_llcp_socket_release")
Cc: stable@vger.kernel.org
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
---
 net/nfc/llcp_sock.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
index feab29fc6..0c00cdaab 100644
--- a/net/nfc/llcp_sock.c
+++ b/net/nfc/llcp_sock.c
@@ -152,6 +152,7 @@ static int llcp_raw_sock_bind(struct socket *sock, struct sockaddr_unsized *addr
 	struct sock *sk = sock->sk;
 	struct nfc_llcp_sock *llcp_sock = nfc_llcp_sock(sk);
 	struct nfc_llcp_local *local;
+	struct nfc_llcp_local *old_local;
 	struct nfc_dev *dev;
 	struct sockaddr_nfc_llcp llcp_addr;
 	int len, ret = 0;
@@ -185,6 +186,11 @@ static int llcp_raw_sock_bind(struct socket *sock, struct sockaddr_unsized *addr
 		goto put_dev;
 	}
 
+	old_local = llcp_sock->local;
+	llcp_sock->local = NULL;
+	llcp_sock->dev = NULL;
+	nfc_llcp_local_put(old_local);
+
 	llcp_sock->dev = dev;
 	llcp_sock->local = local;
 	llcp_sock->nfc_protocol = llcp_addr.nfc_protocol;
-- 
2.43.0

Re: [PATCH net] nfc: llcp: Fix raw socket local ref leak on rebind

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260713012111.4066423-1-shuangpeng.kernel@gmail.com?part=1
---
Now I have enough context to produce the report.

commit 569ae381cd8b4adedc1a30fa7c6777d518fb2eb7
Author: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
Subject: nfc: llcp: Fix raw socket local ref leak on rebind

This patch fixes a leak of the nfc_llcp_local reference owned by raw
LLCP sockets when the bound NFC device is unregistered and the socket
is subsequently rebound. It drops any stale llcp_sock->local before
storing the new reference obtained from nfc_llcp_find_local().

> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index feab29fc62f4..0c00cdaabbc6 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
> @@ -152,6 +152,7 @@ static int llcp_raw_sock_bind(struct socket *sock, struct sockaddr_unsized *addr
>  	struct sock *sk = sock->sk;
>  	struct nfc_llcp_sock *llcp_sock = nfc_llcp_sock(sk);
>  	struct nfc_llcp_local *local;
> +	struct nfc_llcp_local *old_local;
>  	struct nfc_dev *dev;
>  	struct sockaddr_nfc_llcp llcp_addr;
>  	int len, ret = 0;
> @@ -185,6 +186,11 @@ static int llcp_raw_sock_bind(struct socket *sock, struct sockaddr_unsized *addr
>  		goto put_dev;
>  	}
>  
> +	old_local = llcp_sock->local;
> +	llcp_sock->local = NULL;
> +	llcp_sock->dev = NULL;
> +	nfc_llcp_local_put(old_local);
> +
>  	llcp_sock->dev = dev;
>  	llcp_sock->local = local;
>  	llcp_sock->nfc_protocol = llcp_addr.nfc_protocol;

[High]
Does the same rebind leak exist on the non-raw path in llcp_sock_bind()?

nfc_llcp_socket_release() walks local->sockets first and, for every
non-raw socket, sets sk_state to LLCP_CLOSED and calls sk_del_node_init(),
but does not touch llcp_sock->local:

net/nfc/llcp_core.c:nfc_llcp_socket_release() {
    ...
    sk_for_each_safe(sk, tmp, &local->sockets.head) {
        llcp_sock = nfc_llcp_sock(sk);
        ...
        sk->sk_state = LLCP_CLOSED;
        sk->sk_state_change(sk);
        bh_unlock_sock(sk);
        sk_del_node_init(sk);
    }
    ...
    /* If we still have a device, we keep the RAW sockets alive */
    if (device == true)
        return;
    ...
}

That path is reachable for non-raw sockets from nfc_llcp_mac_is_down()
(which calls nfc_llcp_socket_release(local, true, 0)) while the nfc_dev
is still around, so a userspace rebind of a SOCK_STREAM/SOCK_DGRAM LLCP
socket that is now in LLCP_CLOSED with a stale llcp_sock->local passes
the gate and reaches:

net/nfc/llcp_sock.c:llcp_sock_bind() {
    ...
    if (sk->sk_state != LLCP_CLOSED) {
        ret = -EBADFD;
        goto error;
    }
    ...
    local = nfc_llcp_find_local(dev);
    ...
    llcp_sock->dev = dev;
    llcp_sock->local = local;
    ...
    llcp_sock->service_name = kmemdup(llcp_addr.service_name,
                                      llcp_sock->service_name_len,
                                      GFP_KERNEL);
    ...
}

Would this overwrite of llcp_sock->local leak the previous reference
returned by the earlier nfc_llcp_find_local(), together with the nfc_dev
ref that nfc_llcp_local_get() holds via local->dev, in the same way as
the raw path this patch is fixing?

As a secondary consequence in the same window, the unconditional
llcp_sock->service_name = kmemdup(...) reassignment appears to overwrite
the previous kmemdup allocation without a kfree(), and
llcp_sock->reserved_ssap continues to reference an SSAP that was reserved
on the earlier local.

Would it make sense to extend the same "drop old_local before overwriting"
pattern to llcp_sock_bind() so both bind paths are covered by the fix?