Re: [PATCH] nfc: llcp: Fix nfc_dev refcount leak in connect

[David Heidelberg]

On 07/07/2026 20:35, Shuangpeng Bai wrote:
> llcp_sock_connect() takes a reference to the NFC device with
> nfc_get_device() and stores it in llcp_sock->dev before moving the
> socket to LLCP_CONNECTING.
>
> If such a pending connection is released before reaching
> LLCP_CONNECTED, llcp_sock_destruct() does not drop that device
> reference because it only handles connected sockets. This leaks the
> nfc_dev reference acquired during connect.
>
> Drop the device reference for LLCP_CONNECTING sockets as well.
>
> Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@xxxxxxxxx>
> ---
>   net/nfc/llcp_sock.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)

Hello Shuangpeng,

could you check the Sashiko review, if you can address the issue found within 
the patch (the existing code issues are optional to address)?

--- Patch [1]: [PATCH] nfc: llcp: Fix nfc_dev refcount leak in connect ---
commit 84e092575501f812e707c62695cef1ec7ff9d8a6
Author: Shuangpeng Bai <shuangpeng.kernel@xxxxxxxxx>

nfc: llcp: Fix nfc_dev refcount leak in connect

llcp_sock_connect() takes a reference to the NFC device with
nfc_get_device() and stores it in llcp_sock->dev before moving the
socket to LLCP_CONNECTING. If such a pending connection is released
before reaching LLCP_CONNECTED, llcp_sock_destruct() does not drop that
device reference because it only handles connected sockets. This leaks the
nfc_dev reference acquired during connect. Drop the device reference for
LLCP_CONNECTING sockets as well.

> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index feab29fc62f44..44639ccd438e4 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
> @@ -960,7 +960,8 @@ static void llcp_sock_destruct(struct sock *sk)
>
>  	pr_debug("%p\n", sk);
>
> -	if (sk->sk_state == LLCP_CONNECTED)
> +	if (sk->sk_state == LLCP_CONNECTED ||
> +	    sk->sk_state == LLCP_CONNECTING)

This isn't a bug introduced by this patch, but does llcp_sock_connect()
lack a state check to prevent it from being called on sockets in LLCP_BOUND
or LLCP_LISTEN states?

If a user creates an AF_NFC socket and calls bind(), placing the socket in
the LLCP_BOUND state, it links into the local->sockets list via its sk_node.
If the user then calls connect(), which only checks for LLCP_CONNECTED and
LLCP_CONNECTING states:

net/nfc/llcp_sock.c:llcp_sock_connect() {
    if (sk->sk_state == LLCP_CONNECTED) {
        ret = -EISCONN;
        goto error;
    }
    if (sk->sk_state == LLCP_CONNECTING) {
        ret = -EINPROGRESS;
        goto error;
    }
    ...
}

It proceeds to overwrite llcp_sock->service_name (leaking the previously
allocated string) and calls nfc_llcp_sock_link(&local->connecting_sockets,
sk).

This executes hlist_add_head() on a node (sk->sk_node) that is already part
of another list, thereby corrupting the pointers of both the sockets and
connecting_sockets lists.

> +		nfc_put_device(llcp_sock->dev);

Can this introduce a kernel panic via a NULL pointer dereference when a
blocking connect() call fails or is interrupted?

In llcp_sock_connect(), if the wait fails or is interrupted by a signal,
execution jumps to the sock_unlink error path:

net/nfc/llcp_sock.c:llcp_sock_connect() {
    sk->sk_state = LLCP_CONNECTING;

    ret = sock_wait_state(sk, LLCP_CONNECTED,
                          sock_sndtimeo(sk, flags & O_NONBLOCK));
    if (ret && ret != -EINPROGRESS)
        goto sock_unlink;
    ...
sock_llcp_nullify:
    llcp_sock->local = NULL;
    llcp_sock->dev = NULL;
}

This error path nullifies llcp_sock->dev and drops the device reference,
but crucially fails to reset sk->sk_state back to LLCP_CLOSED.

When the user subsequently closes the socket, llcp_sock_destruct() is
invoked. The newly added logic checks for LLCP_CONNECTING sockets and
unconditionally calls nfc_put_device(llcp_sock->dev).

Since dev is NULL, nfc_put_device() evaluates &NULL->dev:

include/net/nfc/nfc.h:nfc_put_device() {
    put_device(&dev->dev);
}

This evaluates to a non-zero struct offset and bypasses the internal NULL
check in put_device(), attempting to decrement the reference count of
unmapped memory.

>
>  	skb_queue_purge(&sk->sk_receive_queue);
>