Re: [PATCH net-next 1/5] tls: reject the combination of TLS and sockmap

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The patch attempts to reject the combination of TLS and sockmap during key configuration, but leaves a critical vulnerability where attaching the TLS ULP to a socket already in a sockmap causes global protocol state corruption, infinite loops in unrelated sockets, and silently bypasses BPF sk_msg policies.
--

commit 12fd2fff4db720c6f124011a38950efdbcd77414
Author: Jakub Kicinski <kuba@kernel.org>

tls: reject the combination of TLS and sockmap

This commit attempts to reject the combination of TLS and sockmap by
returning -EINVAL in do_tls_setsockopt_conf() if a psock is attached.
The goal is to prevent latent bugs while removing dead code.

> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index 13c88a7b8787d..8e7ba018988d2 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -643,6 +643,17 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
[ ... ]
>  	int rc = 0;
>  	int conf;
>  
> +	/* TLS and sockmap are mutually exclusive. A socket already in a
> +	 * sockmap (i.e. with a psock attached) cannot be upgraded to TLS.
> +	 * sockmap rejects TLS sockets already (see sk_psock_init()).
> +	 */
> +	rcu_read_lock();
> +	if (sk_psock(sk)) {
> +		rcu_read_unlock();
> +		return -EINVAL;
> +	}
> +	rcu_read_unlock();
> +
[Severity: Critical]
This isn't a bug introduced by this patch, but does this check happen too
late to prevent global protocol state corruption?

If an unprivileged user adds an established TCP socket to a sockmap, the
socket's sk_prot is changed to tcp_bpf_prots. If they then attach the TLS ULP,
tls_init() executes and immediately calls tls_build_proto() before this
key configuration check is ever reached:

net/tls/tls_main.c:tls_init() {
    ...
    tls_build_proto(sk);
    ...
}

This function reads the socket's current sk_prot and uses it to rebuild the
global tls_prots array:

net/tls/tls_main.c:tls_build_proto() {
    ...
    struct proto *prot = READ_ONCE(sk->sk_prot);
    ...
    build_protos(tls_prots[TLSV4], prot);
    ...
}

Because sk_prot is tcp_bpf_prots, the unhash handler in the tls_prots array 
is permanently overwritten with sock_map_unhash.

Any subsequent normal TLS socket will inherit this corrupted global array.
When the normal TLS socket is closed, sock_map_unhash() is invoked. Since
the normal socket has no psock, it attempts to fall back to the original
unhash function, leading to an infinite retry loop:

net/core/sock_map.c:sock_map_unhash() {
    ...
    saved_unhash = READ_ONCE(sk->sk_prot)->unhash;
    if (unlikely(saved_unhash == sock_map_unhash))
        goto retry;
    ...
}

Because sk->sk_prot points to the corrupted tls_prots array, it reads
sock_map_unhash again, causing a kernel hard lockup.

Could a check be placed in tls_init() or tcp_set_ulp() instead, to prevent
ULP attachment entirely before tls_build_proto() is called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614014102.461064-1-kuba@kernel.org?part=1