1. Patchew
  2. MPTCP
  3. View series

[PATCH mptcp-next] mptcp: bpf: fix NULL derefs in bpf_mptcp_sock_from_subflow()

Kalpan Jani posted 1 patch 1 month, 1 week ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/20260612072643.2313900-1-kalpan.jani@mpiricsoftware.com
net/mptcp/bpf.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
[PATCH mptcp-next] mptcp: bpf: fix NULL derefs in bpf_mptcp_sock_from_subflow()
Posted by Kalpan Jani 1 month, 1 week ago 
bpf_mptcp_sock_from_subflow() is reachable from tracing BPF programs via
bpf_skc_to_mptcp_sock() on an arbitrary socket, without the socket lock
held. It assumes sk_is_mptcp(sk) implies a valid subflow context whose
->conn points to a parent mptcp_sock. That invariant does not hold in
two lifecycle windows:

- Fallback: subflow_ulp_fallback() clears icsk_ulp_data before clearing
  tcp_sk(sk)->is_mptcp, so a concurrent reader can observe is_mptcp == 1
  with a NULL context, dereferencing NULL via mptcp_subflow_ctx(sk)->conn.

- Init: subflow_ulp_init() sets is_mptcp = 1 while ctx->conn is still
  NULL. As mptcp_sk() is a container_of() on a non-zero offset member,
  mptcp_sk(NULL) yields a non-NULL bogus pointer that passes the verifier
  NULL check (RET_PTR_TO_BTF_ID_OR_NULL); on CONFIG_DEBUG_NET the
  mptcp_sk() WARN_ON dereferences it directly.

Load the context once and reject a NULL context or NULL ->conn before
casting.

Fixes: 3bc253c2e652 ("bpf: Add bpf_skc_to_mptcp_sock_proto")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/622
Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
---
 net/mptcp/bpf.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 08bb037f0951..f40905fbe7d4 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -193,8 +193,13 @@ static struct bpf_struct_ops bpf_mptcp_sched_ops = {
 
 struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
 {
-	if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk))
-		return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
+	struct mptcp_subflow_context *ctx;
+
+	if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk)) {
+		ctx = mptcp_subflow_ctx(sk);
+		if (ctx && ctx->conn)
+			return mptcp_sk(ctx->conn);
+	}
 
 	return NULL;
 }
-- 
2.43.0
Re: [PATCH mptcp-next] mptcp: bpf: fix NULL derefs in bpf_mptcp_sock_from_subflow()
Posted by Li Xiasong 3 weeks, 3 days ago 
Hi Kalpan,

I noticed this patch has been on the list for a while, so I took a look
and have a couple of comments.

On 6/12/2026 3:26 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 socket lock
> held. It assumes sk_is_mptcp(sk) implies a valid subflow context whose
> ->conn points to a parent mptcp_sock. That invariant does not hold in
> two lifecycle windows:
> 

This helper can indeed be called without holding the subflow socket lock.

> - Fallback: subflow_ulp_fallback() clears icsk_ulp_data before clearing
>   tcp_sk(sk)->is_mptcp, so a concurrent reader can observe is_mptcp == 1
>   with a NULL context, dereferencing NULL via mptcp_subflow_ctx(sk)->conn.
> 
> - Init: subflow_ulp_init() sets is_mptcp = 1 while ctx->conn is still
>   NULL. As mptcp_sk() is a container_of() on a non-zero offset member,
>   mptcp_sk(NULL) yields a non-NULL bogus pointer that passes the verifier
>   NULL check (RET_PTR_TO_BTF_ID_OR_NULL); on CONFIG_DEBUG_NET the
>   mptcp_sk() WARN_ON dereferences it directly.
> 

These two NULL cases look real to me.

> Load the context once and reject a NULL context or NULL ->conn before
> casting.
> 
> Fixes: 3bc253c2e652 ("bpf: Add bpf_skc_to_mptcp_sock_proto")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/622

I don't think this fully closes #622.  That issue also mentions a stale
ctx->conn / UAF case during teardown, while this patch only addresses the
immediate NULL-deref cases.

> Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
> ---
>  net/mptcp/bpf.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 08bb037f0951..f40905fbe7d4 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -193,8 +193,13 @@ static struct bpf_struct_ops bpf_mptcp_sched_ops = {
>  
>  struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
>  {
> -	if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk))
> -		return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
> +	struct mptcp_subflow_context *ctx;
> +
> +	if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk)) {
> +		ctx = mptcp_subflow_ctx(sk);
> +		if (ctx && ctx->conn)
> +			return mptcp_sk(ctx->conn);
> +	}
>  
>  	return NULL;
>  }

This still seems incomplete for the lockless access pattern used here.

icsk_ulp_data is an __rcu pointer, but mptcp_subflow_ctx() does a plain
forced dereference of it.  Since this helper can run without the socket
lock, this path should use rcu_dereference() or rcu_dereference_check()
when loading the subflow context.

ctx->conn is also checked and then read again:

	if (ctx && ctx->conn)
		return mptcp_sk(ctx->conn);

For a lockless access, it should be loaded once, for example with
READ_ONCE(). However, READ_ONCE(ctx->conn) would only avoid the double
load / NULL case. It would not provide a lifetime guarantee for the
returned parent mptcp_sock.

During teardown, subflow_ulp_release() drops the subflow-owned reference
to the parent mptcp socket with sock_put(sk), and ctx->conn is not
cleared.  I am not sure under what conditions the subflow context could
still be observable by BPF after that point, since this is called from
the subflow socket destruction path.  It would be helpful to clarify
whether such a window can actually be reached, and if so, what protects
the returned parent mptcp_sock.  If the window is real, clearing
ctx->conn before sock_put() and ensuring the parent mptcp_sock is
RCU-safe (or not returning it without an extra reference) might be
needed.

So the added NULL checks look like a useful improvement for the immediate
NULL-deref cases, but I am not sure they fully cover the lockless access
and lifetime aspects of this helper.  It would be good to clarify the
expected lifetime guarantee for the returned parent mptcp_sock, or extend
the fix so that this helper cannot return a parent socket whose
subflow-owned reference may already have been dropped.

Thanks,
Li Xiasong
Re: [PATCH mptcp-next] mptcp: bpf: fix NULL derefs in bpf_mptcp_sock_from_subflow()
Posted by Kalpan Jani 3 weeks, 3 days ago 
Hi Li,

On Thu, 25 Jun 2026 17:10:54 +0530, Li Xiasong wrote:
> This still seems incomplete for the lockless access pattern used here.
>
> icsk_ulp_data is an __rcu pointer, but mptcp_subflow_ctx() does a plain
> forced dereference of it. Since this helper can run without the socket
> lock, this path should use rcu_dereference() or rcu_dereference_check()
> when loading the subflow context.

Agreed. I will replace the mptcp_subflow_ctx() call with:

	ctx = rcu_dereference_check(inet_csk(sk)->icsk_ulp_data,
				    lockdep_sock_is_held(sk));

This satisfies lockdep for both the locked and lockless (tracing BPF)
callers without requiring an unconditional rcu_read_lock() at the call
site.

> ctx->conn is also checked and then read again:
>
>     if (ctx && ctx->conn)
>         return mptcp_sk(ctx->conn);
>
> For a lockless access, it should be loaded once, for example with
> READ_ONCE().

Agreed. I will load it once:

	conn = READ_ONCE(ctx->conn);
	if (!conn)
		return NULL;
	return mptcp_sk(conn);

> I don't think this fully closes #622. That issue also mentions a stale
> ctx->conn / UAF case during teardown, while this patch only addresses
> the immediate NULL-deref cases.

You are right, and I should not have used Closes: for that reason. The
UAF case is real: subflow_ulp_release() calls sock_put(ctx->conn) without
clearing ctx->conn, so a BPF tracing program firing on the subflow
destruction path can observe a non-NULL ctx->conn that points to freed
memory.

I see two approaches to fix it properly:

1. Add WRITE_ONCE(ctx->conn, NULL) in subflow_ulp_release() before the
   sock_put(), so the READ_ONCE() check in this helper catches the
   teardown window. This is the minimal change but I want to confirm with
   the MPTCP maintainers that no other user of ctx->conn in the release
   path relies on it remaining set across the sock_put().

2. Wrap this helper in rcu_read_lock()/rcu_read_unlock() and ensure the
   parent mptcp_sock is freed via call_rcu rather than immediately, so a
   reader inside the critical section is guaranteed a stable object. This
   requires auditing the mptcp_sock free path which I have not done yet.

Since this patch carries a Fixes: tag against an old commit and targets
net, I think the right split is:

  - v2 of this patch: fixes the two NULL-deref cases with
    rcu_dereference_check() and READ_ONCE(), changes Closes: to Link:
    so the issue is referenced without claiming full resolution, and
    updates the commit message to explicitly note the UAF case is known
    and out of scope.

  - A follow-up patch against net-next: addresses the UAF/lifetime
    issue once the correct approach is agreed with maintainers.

Does that split seem reasonable? I am happy to discuss the preferred
approach for the teardown case before sending v2.

Thanks for the careful review.

Cheers,
Kalpan Jani


From: Li Xiasong <lixiasong1@huawei.com>
To: "Kalpan Jani"<kalpan.jani@mpiricsoftware.com>
Cc: <matttbe@kernel.org>, <martineau@kernel.org>, <pabeni@redhat.com>, <shardul.b@mpiricsoftware.com>, <janak@mpiric.us>, <kalpanjani009@gmail.com>, <shardulsb08@gmail.com>, "zhangchangzhong"<zhangchangzhong@huawei.com>, "yuehaibing"<yuehaibing@huawei.com>, "weiyongjun (A)"<weiyongjun1@huawei.com>, "mptcp@lists.linux.dev"<mptcp@lists.linux.dev>
Date: Thu, 25 Jun 2026 17:10:54 +0530
Subject: Re: [PATCH mptcp-next] mptcp: bpf: fix NULL derefs in bpf_mptcp_sock_from_subflow()

 > Hi Kalpan,
 > 
 > I noticed this patch has been on the list for a while, so I took a look
 > and have a couple of comments.
 > 
 > On 6/12/2026 3:26 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 socket lock
 > > held. It assumes sk_is_mptcp(sk) implies a valid subflow context whose
 > > ->conn points to a parent mptcp_sock. That invariant does not hold in
 > > two lifecycle windows:
 > > 
 > 
 > This helper can indeed be called without holding the subflow socket lock.
 > 
 > > - Fallback: subflow_ulp_fallback() clears icsk_ulp_data before clearing
 > >   tcp_sk(sk)->is_mptcp, so a concurrent reader can observe is_mptcp == 1
 > >   with a NULL context, dereferencing NULL via mptcp_subflow_ctx(sk)->conn.
 > > 
 > > - Init: subflow_ulp_init() sets is_mptcp = 1 while ctx->conn is still
 > >   NULL. As mptcp_sk() is a container_of() on a non-zero offset member,
 > >   mptcp_sk(NULL) yields a non-NULL bogus pointer that passes the verifier
 > >   NULL check (RET_PTR_TO_BTF_ID_OR_NULL); on CONFIG_DEBUG_NET the
 > >   mptcp_sk() WARN_ON dereferences it directly.
 > > 
 > 
 > These two NULL cases look real to me.
 > 
 > > Load the context once and reject a NULL context or NULL ->conn before
 > > casting.
 > > 
 > > Fixes: 3bc253c2e652 ("bpf: Add bpf_skc_to_mptcp_sock_proto")
 > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/622
 > 
 > I don't think this fully closes #622.  That issue also mentions a stale
 > ctx->conn / UAF case during teardown, while this patch only addresses the
 > immediate NULL-deref cases.
 > 
 > > Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
 > > ---
 > >  net/mptcp/bpf.c | 9 +++++++--
 > >  1 file changed, 7 insertions(+), 2 deletions(-)
 > > 
 > > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
 > > index 08bb037f0951..f40905fbe7d4 100644
 > > --- a/net/mptcp/bpf.c
 > > +++ b/net/mptcp/bpf.c
 > > @@ -193,8 +193,13 @@ static struct bpf_struct_ops bpf_mptcp_sched_ops = {
 > >  
 > >  struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
 > >  {
 > > -    if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk))
 > > -        return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
 > > +    struct mptcp_subflow_context *ctx;
 > > +
 > > +    if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk)) {
 > > +        ctx = mptcp_subflow_ctx(sk);
 > > +        if (ctx && ctx->conn)
 > > +            return mptcp_sk(ctx->conn);
 > > +    }
 > >  
 > >      return NULL;
 > >  }
 > 
 > This still seems incomplete for the lockless access pattern used here.
 > 
 > icsk_ulp_data is an __rcu pointer, but mptcp_subflow_ctx() does a plain
 > forced dereference of it.  Since this helper can run without the socket
 > lock, this path should use rcu_dereference() or rcu_dereference_check()
 > when loading the subflow context.
 > 
 > ctx->conn is also checked and then read again:
 > 
 >     if (ctx && ctx->conn)
 >         return mptcp_sk(ctx->conn);
 > 
 > For a lockless access, it should be loaded once, for example with
 > READ_ONCE(). However, READ_ONCE(ctx->conn) would only avoid the double
 > load / NULL case. It would not provide a lifetime guarantee for the
 > returned parent mptcp_sock.
 > 
 > During teardown, subflow_ulp_release() drops the subflow-owned reference
 > to the parent mptcp socket with sock_put(sk), and ctx->conn is not
 > cleared.  I am not sure under what conditions the subflow context could
 > still be observable by BPF after that point, since this is called from
 > the subflow socket destruction path.  It would be helpful to clarify
 > whether such a window can actually be reached, and if so, what protects
 > the returned parent mptcp_sock.  If the window is real, clearing
 > ctx->conn before sock_put() and ensuring the parent mptcp_sock is
 > RCU-safe (or not returning it without an extra reference) might be
 > needed.
 > 
 > So the added NULL checks look like a useful improvement for the immediate
 > NULL-deref cases, but I am not sure they fully cover the lockless access
 > and lifetime aspects of this helper.  It would be good to clarify the
 > expected lifetime guarantee for the returned parent mptcp_sock, or extend
 > the fix so that this helper cannot return a parent socket whose
 > subflow-owned reference may already have been dropped.
 > 
 > Thanks,
 > Li Xiasong
 > 
 > 
 >
Re: [PATCH mptcp-next] mptcp: bpf: fix NULL derefs in bpf_mptcp_sock_from_subflow()
Posted by MPTCP CI 1 month, 1 week ago 
Hi Kalpan,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/27402019958

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/910803a3a0e4
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1110481


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.