[PATCH mptcp-net v2] mptcp: fix skb_ext leak in fallback mode

[PATCH mptcp-net v2] mptcp: fix skb_ext leak in fallback mode

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

In fallback mode, MPTCP sockets behave as plain TCP and should not allocate
SKB_EXT_MPTCP for transmitted skbs. The current code unconditionally allocates
the extension, causing memory leaks when skbs are freed without releasing it.

Fix by short-circuiting __mptcp_add_ext() in fallback mode and skipping all
DSS bookkeeping in mptcp_sendmsg_frag(). Also allow TCP coalescing when mpext
is NULL.

This latent bug will be exposed once TLS ULP support is added to fallback
MPTCP sockets, as each sendmsg via the TLS path would leak one skb_ext
object.

Fixes: 3a54a74a3c5b ("mptcp: allocate TX skbs in msk context")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
v2:
 - Free extensions before early returns
 - Added fallback label to skip mpext operations
 - Allow TCP coalescing when mpext NULL
 - Cache fallback state in bool fb

v1:
 - https://patchwork.kernel.org/project/mptcp/patch/70a7e7e05337faa0547c3759e5d9829763f2bcc5.1788244452.git.tanggeliang@kylinos.cn/
---
 net/mptcp/protocol.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0b24e0afedfb..15877aa601ce 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1158,10 +1158,13 @@ static bool mptcp_skb_can_collapse_to(u64 write_seq,
 	if (!tcp_skb_can_collapse_to(skb))
 		return false;
 
+	if (!mpext)
+		return true;
+
 	/* can collapse only if MPTCP level sequence is in order and this
 	 * mapping has not been xmitted yet
 	 */
-	return mpext && mpext->data_seq + mpext->data_len == write_seq &&
+	return mpext->data_seq + mpext->data_len == write_seq &&
 	       !mpext->frozen;
 }
 
@@ -1361,7 +1364,8 @@ static struct sk_buff *__mptcp_do_alloc_tx_skb(struct sock *sk, gfp_t gfp)
 
 	skb = alloc_skb_fclone(MAX_TCP_HEADER, gfp);
 	if (likely(skb)) {
-		if (likely(__mptcp_add_ext(skb, gfp))) {
+		if (unlikely(__mptcp_check_fallback(mptcp_sk(sk))) ||
+		    likely(__mptcp_add_ext(skb, gfp))) {
 			skb_reserve(skb, MAX_TCP_HEADER);
 			skb->ip_summed = CHECKSUM_PARTIAL;
 			INIT_LIST_HEAD(&skb->tcp_tsorted_anchor);
@@ -1438,6 +1442,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	u64 data_seq = dfrag->data_seq + info->sent;
 	int offset = dfrag->offset + info->sent;
 	struct mptcp_sock *msk = mptcp_sk(sk);
+	bool fb = __mptcp_check_fallback(msk);
 	bool zero_window_probe = false;
 	struct mptcp_ext *mpext = NULL;
 	bool can_coalesce = false;
@@ -1507,6 +1512,8 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 		 */
 		if (snd_una != msk->snd_nxt || skb->len ||
 		    skb != tcp_send_head(ssk)) {
+			if (unlikely(fb) && mpext)
+				skb_ext_del(skb, SKB_EXT_MPTCP);
 			tcp_remove_empty_skb(ssk);
 			return 0;
 		}
@@ -1518,6 +1525,8 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 
 	copy = min_t(size_t, copy, info->limit - info->sent);
 	if (!sk_wmem_schedule(ssk, copy)) {
+		if (unlikely(fb) && mpext)
+			skb_ext_del(skb, SKB_EXT_MPTCP);
 		tcp_remove_empty_skb(ssk);
 		return -ENOMEM;
 	}
@@ -1538,6 +1547,15 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	TCP_SKB_CB(skb)->end_seq += copy;
 	tcp_skb_pcount_set(skb, 0);
 
+	/* in fallback mode, skip DSS bookkeeping and free the extension
+	 * if allocated
+	 */
+	if (unlikely(fb)) {
+		if (mpext)
+			skb_ext_del(skb, SKB_EXT_MPTCP);
+		goto fallback;
+	}
+
 	/* on skb reuse we just need to update the DSS len */
 	if (reuse_skb) {
 		TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_PSH;
@@ -1571,6 +1589,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 	if (mptcp_subflow_ctx(ssk)->send_infinite_map)
 		mptcp_update_infinite_map(msk, ssk, mpext);
 	trace_mptcp_sendmsg_frag(mpext);
+fallback:
 	mptcp_subflow_ctx(ssk)->rel_write_seq += copy;
 
 	/* if this is the last chunk of a dfrag with MSG_EOR set,
-- 
2.53.0

Re: [PATCH mptcp-net v2] mptcp: fix skb_ext leak in fallback mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A TOCTOU (Time-Of-Check to Time-Of-Use) race condition between cached fallback state and a dynamic check during SKB allocation leads to a deterministic NULL pointer dereference.
- [High] Unconditional dereference of `mpext` in `mptcp_sendmsg_frag()` after improperly allowing coalescing into SKBs without MPTCP extensions on non-fallback sockets.
--

commit 5ed318da20af8ceeb8c86c1e52fb150843792bb6
Author: Geliang Tang <geliang@kernel.org>

mptcp: fix skb_ext leak in fallback mode

This patch short-circuits __mptcp_add_ext() in fallback mode to avoid
allocating SKB_EXT_MPTCP and skips DSS bookkeeping in mptcp_sendmsg_frag().
It also allows coalescing into extension-less SKBs.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0098e28309318..2e92b24c157bb 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1121,10 +1121,13 @@ static bool mptcp_skb_can_collapse_to(u64 write_seq,
>  	if (!tcp_skb_can_collapse_to(skb))
>  		return false;
>  
> +	if (!mpext)
> +		return true;
> +

[Severity: High]
Does this change mistakenly allow coalescing into extension-less SKBs on
non-fallback sockets?

If an extension-less SKB legitimately exists at the tail of the subflow write
queue (e.g., a SYN-data SKB created by standard TCP Fast Open) and the
connection is not in fallback mode (fb == false):

mptcp_sendmsg_frag() would see mptcp_skb_can_collapse_to() return true, set
reuse_skb = true, and append data.

Because fb is false, the safe fallback routing block is bypassed. Execution
would fall through to the reuse_skb block, where it unconditionally executes
mpext->data_len += copy on a NULL mpext pointer.

Could this trigger a NULL pointer dereference?

[ ... ]
> @@ -1324,7 +1327,8 @@ static struct sk_buff *__mptcp_do_alloc_tx_skb(struct sock *sk, gfp_t gfp)
>  
>  	skb = alloc_skb_fclone(MAX_TCP_HEADER, gfp);
>  	if (likely(skb)) {
> -		if (likely(__mptcp_add_ext(skb, gfp))) {
> +		if (unlikely(__mptcp_check_fallback(mptcp_sk(sk))) ||
> +		    likely(__mptcp_add_ext(skb, gfp))) {
>  			skb_reserve(skb, MAX_TCP_HEADER);

[Severity: High]
Can a race condition occur between the cached fallback state and this dynamic
check?

mptcp_sendmsg_frag() caches the fallback state locally:

	bool fb = __mptcp_check_fallback(msk);

If an incoming packet concurrently triggers fallback and sets
MPTCP_FALLBACK_DONE locklessly via test_and_set_bit(), the dynamic check here
in __mptcp_do_alloc_tx_skb() will observe the new state (true). It will then
omit the SKB extension allocation, leaving mpext as NULL.

Back in mptcp_sendmsg_frag(), because the cached fb is still false, the newly
added fallback block is skipped:

	if (unlikely(fb)) {
		if (mpext)
			skb_ext_del(skb, SKB_EXT_MPTCP);
		goto fallback;
	}

Execution will then fall through and unconditionally dereference the NULL mpext
pointer (e.g., at memset(mpext, 0, sizeof(*mpext))).

Could this lead to a deterministic NULL pointer dereference?

[ ... ]
> @@ -1501,6 +1510,15 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
>  	TCP_SKB_CB(skb)->end_seq += copy;
>  	tcp_skb_pcount_set(skb, 0);
>  
> +	/* in fallback mode, skip DSS bookkeeping and free the extension
> +	 * if allocated
> +	 */
> +	if (unlikely(fb)) {
> +		if (mpext)
> +			skb_ext_del(skb, SKB_EXT_MPTCP);
> +		goto fallback;
> +	}
> +
>  	/* on skb reuse we just need to update the DSS len */
>  	if (reuse_skb) {
>  		TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_PSH;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/b67dec47d321886d45fdd2bca1c303314fcdb229.1788252583.git.tanggeliang@kylinos.cn?part=1

Re: [PATCH mptcp-net v2] mptcp: fix skb_ext leak in fallback mode

[MPTCP CI]

Hi Geliang,

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): Unstable: 1 failed test(s): selftest_simult_flows ⚠️ 
- 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/33491620240

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


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)