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

[David Heidelberg <david@ixit.cz>]

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@gmail.com>
> ---
>   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@gmail.com>

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);
 >