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: