netdev - Re: [PATCH net v2 1/2] bpf, sockmap: Avoid sk_prot reset on sockmap unlink with ULP set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250402230056.6xobt4gwpfnqorzp@gmail.com>
Date: Wed, 2 Apr 2025 16:00:56 -0700
From: John Fastabend <john.fastabend@...il.com>
To: Dong Chenchen <dongchenchen2@...wei.com>
Cc: edumazet@...gle.com, kuniyu@...zon.com, pabeni@...hat.com,
	willemb@...gle.com, jakub@...udflare.com, davem@...emloft.net,
	kuba@...nel.org, horms@...nel.org, daniel@...earbox.net,
	xiyou.wangcong@...il.com, netdev@...r.kernel.org,
	bpf@...r.kernel.org, stfomichev@...il.com, mrpre@....com,
	zhangchangzhong@...wei.com
Subject: Re: [PATCH net v2 1/2] bpf, sockmap: Avoid sk_prot reset on sockmap
 unlink with ULP set

On 2025-03-31 09:21:25, Dong Chenchen wrote:
> WARNING: CPU: 0 PID: 6558 at net/core/sock_map.c:1703 sock_map_close+0x3c4/0x480
> Modules linked in:
> CPU: 0 UID: 0 PID: 6558 Comm: syz-executor.14 Not tainted 6.14.0-rc5+ #238
> RIP: 0010:sock_map_close+0x3c4/0x480
> Call Trace:
>  <TASK>
>  inet_release+0x144/0x280
>  __sock_release+0xb8/0x270
>  sock_close+0x1e/0x30
>  __fput+0x3c6/0xb30
>  __fput_sync+0x7b/0x90
>  __x64_sys_close+0x90/0x120
>  do_syscall_64+0x5d/0x170
>  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> 
> The root cause is:
> bpf_prog_attach(BPF_SK_SKB_STREAM_VERDICT)
> tcp_set_ulp //set ulp after sockmap add
> 	icsk->icsk_ulp_ops = ulp_ops;
> sock_hash_update_common
>   sock_map_unref
>     sock_map_del_link
>       psock->psock_update_sk_prot(sk, psock, false);
> 	sk->sk_prot->close = sock_map_close
> sk_psock_drop
>   sk_psock_restore_proto
>     tcp_bpf_update_proto
>        tls_update //not redo sk_prot to tcp prot
> inet_release
>   sk->sk_prot->close
>     sock_map_close
>       WARN(sk->sk_prot->close == sock_map_close)
> 
> commit e34a07c0ae39 ("sock: redo the psock vs ULP protection check")
> has moved ulp check from tcp_bpf_update_proto() to psock init.
> If sk sets ulp after being added to sockmap, it will reset sk_prot to
> BPF_BASE when removed from sockmap. After the psock is dropped, it will
> not reset sk_prot back to the tcp prot, only tls context update is
> performed. This can trigger a warning in sock_map_close() due to
> recursion of sk->sk_prot->close.
> 
> To fix this issue, skip the sk_prot operations redo when deleting link
> from sockmap if ULP is set.
> 
> Fixes: e34a07c0ae39 ("sock: redo the psock vs ULP protection check")
> Fixes: c0d95d3380ee ("bpf, sockmap: Re-evaluate proto ops when psock is removed from sockmap")
> Suggested-by: Cong Wang <xiyou.wangcong@...il.com>
> Signed-off-by: Dong Chenchen <dongchenchen2@...wei.com>
> ---
> Changes for v2:
> - Move ULP check from sock_map_del_link() to tcp_bpf_update_proto()
> ---
>  net/ipv4/tcp_bpf.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index ba581785adb4..01b3930947cc 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -708,6 +708,9 @@ int tcp_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore)
>  		return 0;
>  	}
>  

I think in this case we also want to update the ULP so that when it
tears down it will restore the correct proto ops?

Either tcp_bpf_update_proto should be called with restore=true or
this logic should be similar to above?

	if (inet_csk_has_ulp(sk)) {
		/* TLS does not have an unhash proto in SW cases,
		 * but we need to ensure we stop using the sock_map
		 * unhash routine because the associated psock is being
		 * removed. So use the original unhash handler.
		 */
		WRITE_ONCE(sk->sk_prot->unhash, psock->saved_unhash);
		tcp_update_ulp(sk, psock->sk_proto, psock->saved_write_space);

> +	if (inet_csk_has_ulp(sk))
> +		return 0;

I'm thinking of this stack of psock/ULPs,

   TCP ops -> sockmap psock -> TLS (ULP) ops

When ULP ops was initialized it stored sockmap ops in its ctx->sk_proto
so that it if it was removed it could restore the correct ops. Similar
sockmap psock stored TCP ops to restore if its removed. So if we remove
psock in the middle we shoudl tell the ULP to update its stored value.

The concerning flow might be,

   socket insert into sockmap (gets psock)
   socket adds ULP
   socket removed from socketmap (removes psock)
   socket removes ULP (restores original tcp ops hopefully)

Perhaps we could also add a test for this case to the 2nd patch?

Thanks,
John