Re: [PATCH mptcp-net v4] mptcp: bpf: don't expose bpf_skc_to_mptcp_sock() to tracing progs

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

Pre-existing issues:
- [Critical] The patch attempts to fix a Use-After-Free / NULL pointer dereference in `bpf_skc_to_mptcp_sock()` by restricting its use to BPF hooks that hold the subflow lock (e.g., `sock_ops`, `cg_sockopt`), thereby removing it from `tracing_prog_func_proto()`. However, the fix is fundamentally incomplete for two reasons. First, the helper remains exported via `bpf_sk_base_func_proto()`, which serves as the default for numerous datapath hooks including XDP (`xdp_func_proto()`) and TC (`tc_cls_act_func_proto()`). Second, even in locked hooks like `sock_ops`, a BPF program can perform a lockless socket lookup (e.g., via `bpf_sk_lookup_tcp()`) and pass the resulting unlocked socket to the helper. Since the helper accepts any `ARG_PTR_TO_SOCK_COMMON` regardless of whether it is the context socket, and the lockless read hardening from v3 was dropped in this patch, this allows the exact same lockless Use-After-Free or NULL pointer dereference on `mptcp_subflow_ctx(sk)->conn` during socket fallback, initialization, or teardown.
--

--- Patch [1]: [PATCH mptcp-net v4] mptcp: bpf: don't expose bpf_skc_to_mptcp_sock() to tracing progs ---
commit 5acf7057a22cd1b7a4c59432f2d0744627b03558
Author: Kalpan Jani <[email protected]>

mptcp: bpf: don't expose bpf_skc_to_mptcp_sock() to tracing progs

The patch drops bpf_skc_to_mptcp_sock() from tracing_prog_func_proto() to
prevent lockless access to mptcp_subflow_ctx(sk)->conn during subflow
fallback, init, and teardown.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/622

> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 75495a5c35079..ad4b8d04b190a 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;

[Severity: Critical]
This is a pre-existing issue, but does removing this helper from tracing
programs completely prevent the lockless dereference regression?

The helper remains exported via bpf_sk_base_func_proto() in net/core/filter.c,
which makes it available to datapath hooks like XDP and TC that do not hold
the subflow lock.

Additionally, even in hooks that do hold the subflow lock for the context
socket (like sock_ops), a BPF program can perform a lockless socket lookup
(e.g., using bpf_sk_lookup_tcp()) and pass that unlocked socket to the helper.

Since the lockless read hardening was dropped from v3 of this patch, the
underlying helper in net/mptcp/bpf.c:bpf_mptcp_sock_from_subflow() still
performs:

    if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk))
        return mptcp_sk(mptcp_subflow_ctx(sk)->conn);

Could this still allow a use-after-free or NULL pointer dereference on
mptcp_subflow_ctx(sk)->conn when an unlocked socket is passed during socket
fallback, initialization, or teardown?

>  	case BPF_FUNC_sk_storage_get:
>  		return &bpf_sk_storage_get_tracing_proto;
>  	case BPF_FUNC_sk_storage_delete:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1