[PATCH mptcp-net v5] mptcp: bpf: fix NULL deref and UAF in bpf_mptcp_sock_from_subflow()

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.

Load the context with rcu_dereference() and read ->conn with
smp_load_acquire(), paired with an smp_store_release() on the publish so
a reader observing a non-NULL ->conn also observes the initialised
parent. Reject a NULL context or NULL ->conn before casting. Clear ->conn
before dropping the parent reference in both release paths, and give the
parent msk RCU-grace lifetime by setting SOCK_RCU_FREE in
__mptcp_init_sock().

Commit 5e20087d1b67 ("mptcp: handle mptcp listener destruction via rcu")
already makes the listener msk SOCK_RCU_FREE for its lockless ->conn
reader and resets the flag on the accepted child, which had no such
reader. This helper adds one to the accepted msk; __mptcp_destroy_sock()
performs all msk teardown before the final sock_put() and shares
mptcp_destroy_common() with the already-RCU-freed listener, so extending
the same lifetime is safe. Drop the reset.

v4 went with dropping this from tracing_prog_func_proto() instead,
since sock_ops/cg_sockopt hold the lock and nothing else uses it from
tracing. Sashiko pointed out the helper's still reachable lock-free
through bpf_sk_base_func_proto() (XDP, TC), and sock_ops can pass in
an arbitrary socket via bpf_sk_lookup_tcp() anyway, so that didn't
actually fix it. Back to hardening the read itself. Tracing stays
removed since nothing needs it there.

Suggested-by: Paolo Abeni <[email protected]>
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 <[email protected]>
---

Changes in v5:
- Sashiko caught that dropping the helper from tracing_prog_func_proto()
in v4 doesn't actually close this - it's still reachable lock-free via
bpf_sk_base_func_proto() (XDP/TC), and sock_ops can pass an arbitrary
socket in via bpf_sk_lookup_tcp() regardless of its own lock. Back to
the v3 lockless-read fix (rcu_dereference/acquire-release/SOCK_RCU_FREE),
on top of keeping the tracing removal from v4.
- Checked if the v3 sleepable guard on the tracing case is still needed
now that tracing access is gone entirely - it's not, none of the
remaining callers (TC, XDP, sock_ops, cg_sockopt, sk_lookup) can load
as sleepable per can_be_sleepable() in the verifier.

Changes in v4:
- Instead of hardening the lockless read (rcu_dereference/acquire-
release/SOCK_RCU_FREE from v3), just drop tracing's access to the
helper, per Paolo. sock_ops and cg_sockopt already hold the subflow
lock so they're not affected, and nothing in-tree uses this from a
tracing program anyway. Patch is now a 2-line removal in
kernel/trace/bpf_trace.c, everything else from v3 is dropped.

Changes in v3:
- Fix the publish race in subflow_syn_recv_sock() that v2 missed
(Sashiko): the passive MPC path stored the freshly cloned parent into
->conn with a plain assignment on an already-hashed child. Publish with
smp_store_release() and pair the helper read with smp_load_acquire(), so
a lockless reader observing a non-NULL ->conn also sees the initialised
parent.
- Clear ->conn before sock_put() in mptcp_subflow_drop_ctx() too, not just
subflow_ulp_release(): it had the same stale-pointer teardown pattern.
- Audited the SOCK_RCU_FREE change: __mptcp_destroy_sock() does all msk
teardown before the final sock_put() and shares mptcp_destroy_common()
with the already-RCU-freed listener, so deferring the free is safe.

Changes in v2:
- Lockless access fixes (Li Xiasong): load the subflow context with
rcu_dereference_check() instead of a plain dereference, and read ->conn
once.
- Fix the teardown use-after-free that v1 did not address: clear ->conn
before dropping the parent reference in subflow_ulp_release(), and give
the parent msk RCU-grace lifetime via SOCK_RCU_FREE.
- Stop exposing the helper to sleepable BPF programs, where classic RCU
gives no lifetime guarantee.

v1: https://lore.kernel.org/all/[email protected]/
v2: https://lore.kernel.org/all/[email protected]/
v3: https://lore.kernel.org/all/[email protected]/
v4: https://lore.kernel.org/all/[email protected]/

---
 kernel/trace/bpf_trace.c |  2 --
 net/mptcp/bpf.c          | 17 +++++++++++++++--
 net/mptcp/protocol.c     |  7 ++++++-
 net/mptcp/subflow.c      | 40 +++++++++++++++++++++++++++++++++-------
 4 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 82f8feea6931..43a5517fde47 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:
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 0845061ddc65..272bfbaf199d 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -193,8 +193,21 @@ 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;
+	struct sock *conn;
+
+	if (sk && sk_fullsock(sk) && sk_is_tcp(sk) && sk_is_mptcp(sk)) {
+		ctx = rcu_dereference(inet_csk(sk)->icsk_ulp_data);
+		if (ctx) {
+			/*
+			 * Pairs with smp_store_release() when ->conn is
+			 * published in subflow_syn_recv_sock().
+			 */
+			conn = smp_load_acquire(&ctx->conn);
+			if (conn)
+				return mptcp_sk(conn);
+		}
+	}
 
 	return NULL;
 }
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ffcf5a1788f6..4c5091c0375c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3182,6 +3182,12 @@ static void __mptcp_init_sock(struct sock *sk)
 	mptcp_pm_data_init(msk);
 	spin_lock_init(&msk->fallback_lock);
 
+	/*
+	 * The returned parent msk may be handed to lockless readers
+	 * (bpf_skc_to_mptcp_sock()); free it after an RCU grace period.
+	 */
+	sock_set_flag(sk, SOCK_RCU_FREE);
+
 	/* re-use the csk retrans timer for MPTCP-level retrans */
 	timer_setup(&sk->mptcp_retransmit_timer, mptcp_retransmit_timer, 0);
 	timer_setup(&msk->sk.mptcp_tout_timer, mptcp_tout_timer, 0);
@@ -3717,7 +3723,6 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
 	/* passive msk is created after the first/MPC subflow */
 	msk->subflow_id = 2;
 
-	sock_reset_flag(nsk, SOCK_RCU_FREE);
 	security_inet_csk_clone(nsk, req);
 
 	/* this can't race with mptcp_close(), as the msk is
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8e386899ceb9..76a649d2c2b6 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -787,8 +787,12 @@ void mptcp_subflow_drop_ctx(struct sock *ssk)
 	list_del(&mptcp_subflow_ctx(ssk)->node);
 	if (inet_csk(ssk)->icsk_ulp_ops) {
 		subflow_ulp_fallback(ssk, ctx);
-		if (ctx->conn)
-			sock_put(ctx->conn);
+		if (ctx->conn) {
+			struct sock *conn = ctx->conn;
+
+			WRITE_ONCE(ctx->conn, NULL);
+			sock_put(conn);
+		}
 	}
 
 	kfree_rcu(ctx, rcu);
@@ -818,6 +822,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	bool fallback, fallback_is_fatal;
 	enum sk_rst_reason reason;
 	struct mptcp_sock *owner;
+	struct sock *conn;
 	struct sock *child;
 
 	pr_debug("listener=%p, req=%p, conn=%p\n", listener, req, listener->conn);
@@ -880,12 +885,19 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		ctx->setsockopt_seq = listener->setsockopt_seq;
 
 		if (ctx->mp_capable) {
-			ctx->conn = mptcp_sk_clone_init(listener->conn, &mp_opt, child, req);
-			if (!ctx->conn)
+			conn = mptcp_sk_clone_init(listener->conn, &mp_opt, child, req);
+			if (!conn)
 				goto fallback;
 
 			ctx->subflow_id = 1;
-			owner = mptcp_sk(ctx->conn);
+			owner = mptcp_sk(conn);
+
+			/*
+			 * Publish the fully initialized parent. Pairs with
+			 * smp_load_acquire() in
+			 * bpf_mptcp_sock_from_subflow().
+			 */
+			smp_store_release(&ctx->conn, conn);
 
 			if (mp_opt.deny_join_id0)
 				WRITE_ONCE(owner->pm.remote_deny_join_id0, true);
@@ -920,7 +932,11 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 
 			/* move the msk reference ownership to the subflow */
 			subflow_req->msk = NULL;
-			ctx->conn = (struct sock *)owner;
+			/*
+			 * Publish the parent. Pairs with smp_load_acquire()
+			 * in bpf_mptcp_sock_from_subflow().
+			 */
+			smp_store_release(&ctx->conn, (struct sock *)owner);
 
 			if (subflow_use_different_sport(owner, sk)) {
 				pr_debug("ack inet_sport=%d %d\n",
@@ -1827,7 +1843,11 @@ int mptcp_subflow_create_socket(struct sock *sk, unsigned short family,
 
 	*new_sock = sf;
 	sock_hold(sk);
-	subflow->conn = sk;
+	/*
+	 * Publish the parent. Pairs with smp_load_acquire() in
+	 * bpf_mptcp_sock_from_subflow().
+	 */
+	smp_store_release(&subflow->conn, sk);
 	mptcp_subflow_ops_override(sf->sk);
 
 	return 0;
@@ -2024,6 +2044,12 @@ static void subflow_ulp_release(struct sock *ssk)
 		if (!release && !test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW,
 						  &mptcp_sk(sk)->flags))
 			mptcp_schedule_work(sk);
+
+		/*
+		 * Hide the parent from lockless readers (e.g. BPF)
+		 * before dropping the subflow-owned reference.
+		 */
+		WRITE_ONCE(ctx->conn, NULL);
 		sock_put(sk);
 	}
 
-- 
2.43.0