Re: [PATCH 1/2] nfc: llcp: Fix use-after-free in llcp_sock_release()

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
nfc: llcp: Fix use-after-free in llcp_sock_release()

This commit addresses a use-after-free issue in llcp_sock_release(). It
attempts to fix the problem by checking if the socket is in the connecting
state, and if so, unlinks it from the connecting_sockets list instead of the
default sockets list.

> diff --git a/net/nfc/llcp_sock.c b/net/nfc/llcp_sock.c
> index f1be1e84f665..feab29fc62f4 100644
> --- a/net/nfc/llcp_sock.c
> +++ b/net/nfc/llcp_sock.c
[ ... ]
> @@ -633,6 +633,8 @@ static int llcp_sock_release(struct socket *sock)
>  
>  	if (sock->type == SOCK_RAW)
>  		nfc_llcp_sock_unlink(&local->raw_sockets, sk);
> +	else if (sk->sk_state == LLCP_CONNECTING)
> +		nfc_llcp_sock_unlink(&local->connecting_sockets, sk);

Does this cover the case where a remote peer rejects a non-blocking
connection with a DM frame?

If a non-blocking connection is initiated, the socket is placed on
local->connecting_sockets with the state LLCP_CONNECTING. If the remote
peer then rejects the connection via a Disconnect Mode (DM) frame,
nfc_llcp_recv_dm() sets sk->sk_state = LLCP_CLOSED but leaves the socket
in the connecting_sockets list.

When llcp_sock_release() later checks the state, it will be LLCP_CLOSED,
causing the new LLCP_CONNECTING check to fail. The code will then fall
through to the else branch:

>  	else
>  		nfc_llcp_sock_unlink(&local->sockets, sk);

Could this corrupt the list? The nfc_llcp_sock_unlink() call for
local->sockets acquires local->sockets.lock and executes sk_del_node_init().
Since the socket is physically still on the local->connecting_sockets hlist,
this would mutate the connecting_sockets list pointers concurrently without
holding the correct local->connecting_sockets.lock.

Simon says: while I think the problem above warrants looking into in
            the context of this patch, I think the problem below pre-dates
	    this patch and does not need to block progress of it.

Is there also a race condition here with nfc_llcp_recv_cc()?

In nfc_llcp_recv_cc(), a connecting socket is unlinked from
connecting_sockets and linked to sockets without acquiring lock_sock(sk).

If llcp_sock_release() executes concurrently (since it holds lock_sock(sk)
while recv_cc does not), could it fully unlink the socket and drop its
references while nfc_llcp_recv_cc() resumes, blindly linking the now
released socket into local->sockets?

This would leave a dangling pointer in the local->sockets list. I noticed
this specific race appears to be addressed later in the series by commit
e4b0d26ffad8cf0db2a59991b8b7098890b74187 ("nfc: llcp: Fix use-after-free
race in nfc_llcp_recv_cc()").