Re: [PATCH mptcp-next 1/2] tls: implement tls_disconnect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] net/tls: double free of tls_context on concurrent connect(AF_UNSPEC)
- [High] net/tls: memory leak of TLS_HW contexts due to unconditional sk_destruct overwrite
- [High] net/tls: NULL pointer dereference in lockless socket ops on concurrent disconnect
- [High] tls: UAF in recvmsg/sendmsg due to concurrent tls_disconnect
- [High] tls: memory leak of HW offload context due to unconditional sk_destruct restore
--

commit c85d98242c0f64ea4b94701ff52f11b27b61f2a3
Author: Geliang Tang <geliang@kernel.org>

tls: implement tls_disconnect

Implement disconnect support for TLS sockets via connect(AF_UNSPEC).
The handler invokes the underlying disconnect() first, then on success
cancels TX work (for TLS_SW), calls tls_sk_proto_free() to restore
original protocol operations and clear TLS state, cleans up RX context
and frees the TLS context.

> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index 8bdb78718a8d4..d00e56af628a0 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c

[ ... ]

> -static void tls_sk_proto_close(struct sock *sk, long timeout)
> +static void tls_sk_proto_free(struct sock *sk,
> +			      struct tls_context *ctx,
> +			      bool free_ctx)
>  {
>  	struct inet_connection_sock *icsk = inet_csk(sk);
> -	struct tls_context *ctx = tls_get_ctx(sk);
>  	long timeo = sock_sndtimeo(sk, 0);
> -	bool free_ctx;
> -
> -	if (ctx->tx_conf == TLS_SW)
> -		tls_sw_cancel_work_tx(ctx);
> -
> -	lock_sock(sk);
> -	free_ctx = ctx->tx_conf != TLS_HW && ctx->rx_conf != TLS_HW;
>  
>  	if (ctx->tx_conf != TLS_BASE || ctx->rx_conf != TLS_BASE)
>  		tls_sk_proto_cleanup(sk, ctx, timeo);
>  
>  	write_lock_bh(&sk->sk_callback_lock);
> -	if (free_ctx)
> -		rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
>  	WRITE_ONCE(sk->sk_prot, ctx->sk_proto);
> +	if (sk->sk_socket)
> +		WRITE_ONCE(sk->sk_socket->ops, ctx->sk_proto_ops);
>  	if (sk->sk_write_space == tls_write_space)
>  		sk->sk_write_space = ctx->sk_write_space;
> +	if (sk->sk_destruct == tls_device_sk_destruct)
> +		sk->sk_destruct = ctx->sk_destruct;

[Severity: High]
For hardware-offloaded TLS sockets (TLS_HW), free_ctx evaluates to false. This
means tls_ctx_free() is intentionally skipped so the context can be cleaned up
asynchronously later.

However, this code unconditionally unhooks tls_device_sk_destruct and restores
the original socket destructor. Does this completely bypass the hardware
offload cleanup and permanently leak the tls_context and associated NIC
hardware state when the socket is ultimately destroyed?

> +	if (free_ctx) {
> +		rcu_assign_pointer(icsk->icsk_ulp_data, NULL);

[Severity: High]
When tls_disconnect() is called, it can run concurrently with lockless
operations like setsockopt() or poll() on the same file descriptor.

If tls_disconnect() sets icsk_ulp_data to NULL here while tls_setsockopt() or
tls_sk_poll() are fetching it locklessly via tls_get_ctx(), won't those
functions dereference a NULL pointer since they don't validate the pointer
before dereferencing ctx?

> +		if (icsk->icsk_ulp_ops) {
> +			/* Balance try_module_get in __tcp_ulp_find_autoload */
> +			module_put(icsk->icsk_ulp_ops->owner);
> +			icsk->icsk_ulp_ops = NULL;
> +		}
> +	}
>  	write_unlock_bh(&sk->sk_callback_lock);
> -	release_sock(sk);
>  	if (ctx->tx_conf == TLS_SW)
>  		tls_sw_free_ctx_tx(ctx);
> +}

[ ... ]

> @@ -904,7 +943,52 @@ static int tls_setsockopt(struct sock *sk, int level, int optname,
>  
>  static int tls_disconnect(struct sock *sk, int flags)
>  {
> -	return -EOPNOTSUPP;
> +	struct tls_context *ctx;
> +	bool free_ctx;
> +	int rc;
> +
> +	ctx = tls_get_ctx(sk);
> +	if (!ctx)
> +		return 0;
> +
> +	if (!ctx->sk_proto->disconnect)
> +		return -EOPNOTSUPP;
> +
> +	set_bit(TLS_DISCONNECTING, &ctx->flags);

[Severity: High]
Two threads can concurrently call connect() with AF_UNSPEC on the same socket
file descriptor.

If thread 1 enters tls_disconnect(), sets the TLS_DISCONNECTING bit, and
temporarily drops the socket lock to wait for disable_delayed_work_sync(),
thread 2 can acquire the lock and enter tls_disconnect() as well.

Since set_bit() is used instead of test_and_set_bit() to abort, thread 2 will
resolve the same ctx pointer and proceed. Will this result in both threads
sequentially executing tls_sk_proto_free() and tls_ctx_free(), causing a
double free of the contexts?

> +
> +	rc = ctx->sk_proto->disconnect(sk, flags);
> +	if (rc) {
> +		clear_bit(TLS_DISCONNECTING, &ctx->flags);
> +		return rc;
> +	}

[ ... ]

> +	if (ctx->rx_conf == TLS_SW) {
> +		struct tls_sw_context_rx *sw_ctx = tls_sw_ctx_rx(ctx);
> +
> +		release_sock(sk);
> +		tls_strp_done(&sw_ctx->strp);
> +		tls_sw_free_ctx_rx(ctx);
> +		lock_sock(sk);
> +	}
> +
> +	if (free_ctx)
> +		tls_ctx_free(sk, ctx);

[Severity: High]
Threads calling tls_sw_recvmsg() or tls_sw_sendmsg() cache pointers to the
RX/TX contexts and then temporarily drop the socket lock to wait for data or
memory via sk_wait_event() or sk_stream_wait_memory().

If a concurrent disconnect triggers tls_disconnect(), it acquires the
socket lock and synchronously frees the RX and TX contexts here.

When the waiting recvmsg or sendmsg thread wakes up, won't it re-acquire the
socket lock and use its locally cached context pointer, leading to a
use-after-free read or write?

> +
> +	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791967771.git.tanggeliang@kylinos.cn?part=1