Re: [PATCH v2 bpf 3/6] mptcp: bpf: fix type confusion in bpf_mptcp_sock_from_subflow()

Re: [PATCH v2 bpf 3/6] mptcp: bpf: fix type confusion in bpf_mptcp_sock_from_subflow()

[Kuniyuki Iwashima]

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

Re: [PATCH v2 bpf 3/6] mptcp: bpf: fix type confusion in bpf_mptcp_sock_from_subflow()

[Matthieu Baerts]

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.