* Re: [PATCH v2 bpf 3/6] mptcp: bpf: fix type confusion in bpf_mptcp_sock_from_subflow() [not found] ` <20260505041435.187D0C2BCB4@smtp.kernel.org> @ 2026-05-05 5:43 ` Kuniyuki Iwashima 2026-05-05 15:14 ` Matthieu Baerts 0 siblings, 1 reply; 2+ messages in thread From: Kuniyuki Iwashima @ 2026-05-05 5:43 UTC (permalink / raw) To: sashiko, Matthieu Baerts; +Cc: bpf Hi Matt, Could you follow up on the comments by Sashiko ? On Mon, May 4, 2026 at 9:14 PM <sashiko-bot@kernel.org> wrote: > > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: > - [High] NULL pointer dereferences in bpf_mptcp_sock_from_subflow due to lockless concurrent accesses > - [High] Use-After-Free in BPF programs returning freed mptcp socket during teardown > -- > > commit 52dbcdaad7d49ff5db2faacf11c47b1c581024a8 > Author: Matthieu Baerts (NGI0) <matttbe@kernel.org> > mptcp: bpf: fix type confusion in bpf_mptcp_sock_from_subflow() > > This commit fixes a type confusion issue in bpf_mptcp_sock_from_subflow() by > checking if the socket is actually a TCP socket before calling sk_is_mptcp(). > Previously, raw sockets could bypass the IPPROTO_TCP check and cause incorrect > assumptions about the socket type. > > > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c > > index 8a16672b94e2..4cc16cbeb328 100644 > > --- a/net/mptcp/bpf.c > > +++ b/net/mptcp/bpf.c > > @@ -14,7 +14,7 @@ > > > > struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk) > > { > > - if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk)) > > + if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk)) > > return mptcp_sk(mptcp_subflow_ctx(sk)->conn); > > Could this cause a regression by dereferencing a NULL pointer when accessed > concurrently? > > In subflow_ulp_fallback(), icsk->icsk_ulp_data is set to NULL prior to > clearing tcp_sk(sk)->is_mptcp. If a BPF program triggers in this window, > sk_is_mptcp() would return true but mptcp_subflow_ctx(sk) would return NULL, > leading to a NULL pointer dereference. > > Additionally, could ctx->conn be NULL when read here? > > During subflow_ulp_init(), the context is allocated and is_mptcp is set to 1, > but ctx->conn is initially NULL. Passing NULL to mptcp_sk(ctx->conn) triggers > a kernel panic on CONFIG_DEBUG_NET builds. On non-debug builds, it returns a > non-NULL offset pointer, which bypasses BPF verifier NULL checks and causes > invalid memory accesses. > > Could this introduce a regression with a use-after-free of the mptcp_sock > during teardown? > > During subflow_ulp_release(), sock_put(ctx->conn) drops the reference to the > parent mptcp_sock and may free it. However, ctx->conn is not cleared, and the > subflow context remains valid until the RCU grace period ends. > > If a BPF tracing program triggers after sock_put(), could this retrieve > and return the dangling ctx->conn pointer, enabling the BPF program to read > from freed kernel slab memory? > > > > > return NULL; > > } > > -- > Sashiko AI review · https://sashiko.dev/#/patchset/20260504210610.180150-1-kuniyu@google.com?part=3 ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2 bpf 3/6] mptcp: bpf: fix type confusion in bpf_mptcp_sock_from_subflow() 2026-05-05 5:43 ` [PATCH v2 bpf 3/6] mptcp: bpf: fix type confusion in bpf_mptcp_sock_from_subflow() Kuniyuki Iwashima @ 2026-05-05 15:14 ` Matthieu Baerts 0 siblings, 0 replies; 2+ messages in thread From: Matthieu Baerts @ 2026-05-05 15:14 UTC (permalink / raw) To: Kuniyuki Iwashima, sashiko; +Cc: bpf, MPTCP Linux Hi Kuniyuki, (+cc MPTCP ML) On 05/05/2026 07:43, Kuniyuki Iwashima wrote: > Could you follow up on the comments by Sashiko ? Thank you for having forwarded this! > On Mon, May 4, 2026 at 9:14 PM <sashiko-bot@kernel.org> wrote: (...) >>> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c >>> index 8a16672b94e2..4cc16cbeb328 100644 >>> --- a/net/mptcp/bpf.c >>> +++ b/net/mptcp/bpf.c >>> @@ -14,7 +14,7 @@ >>> >>> struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk) >>> { >>> - if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk)) >>> + if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk)) >>> return mptcp_sk(mptcp_subflow_ctx(sk)->conn); >> >> Could this cause a regression by dereferencing a NULL pointer when accessed >> concurrently? >> >> In subflow_ulp_fallback(), icsk->icsk_ulp_data is set to NULL prior to >> clearing tcp_sk(sk)->is_mptcp. If a BPF program triggers in this window, >> sk_is_mptcp() would return true but mptcp_subflow_ctx(sk) would return NULL, >> leading to a NULL pointer dereference. This would need to be confirmed, but from what I see, that's not a new bug introduced by the patch here. >> Additionally, could ctx->conn be NULL when read here? >> >> During subflow_ulp_init(), the context is allocated and is_mptcp is set to 1, >> but ctx->conn is initially NULL. Passing NULL to mptcp_sk(ctx->conn) triggers >> a kernel panic on CONFIG_DEBUG_NET builds. On non-debug builds, it returns a >> non-NULL offset pointer, which bypasses BPF verifier NULL checks and causes >> invalid memory accesses. Same here: not a new bug. >> Could this introduce a regression with a use-after-free of the mptcp_sock >> during teardown? >> >> During subflow_ulp_release(), sock_put(ctx->conn) drops the reference to the >> parent mptcp_sock and may free it. However, ctx->conn is not cleared, and the >> subflow context remains valid until the RCU grace period ends. >> >> If a BPF tracing program triggers after sock_put(), could this retrieve >> and return the dangling ctx->conn pointer, enabling the BPF program to read >> from freed kernel slab memory? Same here: not a new bug. Sashiko is good at finding potential existing bugs that predate a patch, which is good, but someone has to fix them :) Because the issues raised here are not new, I suggest not blocking this patch. I will check if someone can look at it quickly. Cheers, Matt -- Sponsored by the NGI0 Core fund. ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-05-05 15:14 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260504210610.180150-4-kuniyu@google.com>
[not found] ` <20260505041435.187D0C2BCB4@smtp.kernel.org>
2026-05-05 5:43 ` [PATCH v2 bpf 3/6] mptcp: bpf: fix type confusion in bpf_mptcp_sock_from_subflow() Kuniyuki Iwashima
2026-05-05 15:14 ` Matthieu Baerts
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox