Re: [PATCH mptcp-net v3] mptcp: bpf: fix NULL deref and UAF in bpf_mptcp_sock_from_subflow()

Paolo Abeni <[email protected]>
Newsgroups dev.linux.lists.mptcp
Message-ID <[email protected]>
On 6/29/26 12:50 PM, Kalpan Jani wrote:
> bpf_mptcp_sock_from_subflow() is reachable from tracing BPF programs via
> bpf_skc_to_mptcp_sock() on an arbitrary socket, without the subflow
> socket lock held. It assumes sk_is_mptcp(sk) implies a valid subflow
> context whose ->conn points to a live parent mptcp_sock. That invariant
> does not hold in several windows:
> 
> - Fallback: subflow_ulp_fallback() clears icsk_ulp_data before clearing
>   tcp_sk(sk)->is_mptcp, so a reader can observe is_mptcp == 1 with a NULL
>   context and dereference mptcp_subflow_ctx(sk)->conn through NULL.
> 
> - Init: subflow_ulp_init() sets is_mptcp = 1 while ->conn is still NULL;
>   on CONFIG_DEBUG_NET, mptcp_sk() dereferences its argument in a
>   WARN_ON(), so mptcp_sk(NULL) faults.
> 
> - Publish: the passive MPC path in subflow_syn_recv_sock() stored the
>   freshly cloned parent into ->conn with a plain assignment, on a child
>   that is already hashed and globally visible. A lockless reader could
>   observe a non-NULL ->conn before the stores initialising the new
>   mptcp_sock were visible.
> 
> - Teardown: subflow_ulp_release() and mptcp_subflow_drop_ctx() dropped
>   the subflow-owned parent reference with sock_put() without clearing
>   ->conn, and the established parent msk was not SOCK_RCU_FREE, so a
>   lockless reader could dereference a freed parent: a use-after-free.

I think we should expose to tracing program arbitrary socket state
locklessly.

@Geliang: what about just removing such possibily? Something alike the 
following (completely untested).

If a tracing bpf program needs to access the msk, it should do just:

	mptcp_sk(sk)

on the main socket, via a new, to be defined/implemented helper, and
_non_ starting form a subflow.

AFAICS all the other accessors using from tracing are simple casts,
like the above, and do not traverse the rather risky ulp layer.

/P
---
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 75495a5c3507..ad4b8d04b190 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1745,8 +1745,6 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 		return &bpf_skc_to_udp6_sock_proto;
 	case BPF_FUNC_skc_to_unix_sock:
 		return &bpf_skc_to_unix_sock_proto;
-	case BPF_FUNC_skc_to_mptcp_sock:
-		return &bpf_skc_to_mptcp_sock_proto;
 	case BPF_FUNC_sk_storage_get:
 		return &bpf_sk_storage_get_tracing_proto;
 	case BPF_FUNC_sk_storage_delete:
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.