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, since DSS mappings are never decoded by
the receiver. However, the current sendmsg path unconditionally allocates
the extension, leading to a leak when the skb is freed without properly
releasing the extension.

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.

Fix this by short-circuiting __mptcp_add_ext() in __mptcp_do_alloc_tx_skb()
when the msk is in fallback mode. In mptcp_sendmsg_frag(), skip all DSS
bookkeeping (memset, data_len updates, frozen flag, csum, infinite_map)
when fallback is set, allowing the subflow to behave like a plain TCP
socket.

Fixes: 3a54a74a3c5b ("mptcp: allocate TX skbs in msk context")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 0b24e0afedfb..96933a221eff 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1361,7 +1361,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);
@@ -1538,6 +1539,12 @@ 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 the msk ext is not allocated; skip DSS bookkeeping
+	 * entirely and let the subflow behave like a plain TCP socket
+	 */
+	if (__mptcp_check_fallback(msk))
+		goto out;
+
 	/* on skb reuse we just need to update the DSS len */
 	if (reuse_skb) {
 		TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_PSH;
-- 
2.53.0

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): Critical: Unexpected stop of the VM - Notice: Call Traces at boot time, rebooted and continued ❌
- KVM Validation: normal (only selftest_mptcp_join): Critical: Unexpected stop of the VM - Notice: Call Traces at boot time, rebooted and continued ❌
- KVM Validation: debug (except selftest_mptcp_join): Unstable: 1 failed test(s): selftest_diag - Critical: Global Timeout - Notice: Call Traces at boot time, rebooted and continued ❌
- KVM Validation: debug (only selftest_mptcp_join): Critical: Unexpected stop of the VM - Notice: Call Traces at boot time, rebooted and continued ❌
- 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/33479854147

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


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)

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Skipping SKB extension allocation in fallback mode leads to guaranteed NULL pointer dereferences in `mptcp_update_data_checksum()` (if checksums are enabled) and `trace_mptcp_sendmsg_frag()`.
- [High] Bypassing SKB extension allocation in fallback mode breaks TCP SKB coalescing, forcing a new SKB allocation for every data fragment.
- [High] A concurrent transition to fallback mode during `mptcp_sendmsg_frag` leaves the SKB MPTCP extension uninitialized, leading to a kernel memory info leak via tracepoints and TCP options.
--

commit 927a3dc5473de2a265a72f67b6be56f765573a1c
Author: Geliang Tang <geliang@kernel.org>
mptcp: fix skb_ext leak in fallback mode

This commit attempts to fix an SKB extension leak when operating in fallback
mode by short-circuiting the SKB_EXT_MPTCP allocation for transmitted skbs. It
also skips DSS bookkeeping in mptcp_sendmsg_frag() for fallback sockets.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0098e28309318..825c71dc4d92a 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1324,7 +1324,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))) {

[Severity: High]
Does skipping the SKB extension allocation in fallback mode break TCP SKB
coalescing?

When mptcp_sendmsg_frag() attempts to coalesce data into the tail SKB, it
calls mptcp_skb_can_collapse_to(). This helper requires a valid mpext and
returns false when it is NULL:

net/mptcp/protocol.c:mptcp_skb_can_collapse_to() {
	if (!tcp_skb_can_collapse_to(skb))
		return false;

	return mpext && mpext->data_seq + mpext->data_len == data_seq &&
	       !mpext->frozen;
}

If mpext is NULL, it forces a new SKB allocation for every chunk and marks
the previous SKB with eor=1 in mptcp_sendmsg_frag(), preventing TCP layer
merging:

net/mptcp/protocol.c:mptcp_sendmsg_frag() {
		mpext = mptcp_get_ext(skb);
		if (!mptcp_skb_can_collapse_to(data_seq, skb, mpext)) {
			TCP_SKB_CB(skb)->eor = 1;
			tcp_mark_push(tcp_sk(ssk), skb);
			goto alloc_skb;
		}
}

>  			skb_reserve(skb, MAX_TCP_HEADER);
>  			skb->ip_summed = CHECKSUM_PARTIAL;
>  			INIT_LIST_HEAD(&skb->tcp_tsorted_anchor);

[ ... ]

> @@ -1501,6 +1502,12 @@ 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 the msk ext is not allocated; skip DSS bookkeeping
> +	 * entirely and let the subflow behave like a plain TCP socket
> +	 */
> +	if (__mptcp_check_fallback(msk))
> +		goto out;
> +

[Severity: Critical]
Will this lead to a NULL pointer dereference at the out label?

Because the SKB extension is not allocated for fallback sockets, mpext will
evaluate to NULL. Jumping to the out label executes this code:

net/mptcp/protocol.c:mptcp_sendmsg_frag() {
out:
	if (READ_ONCE(msk->csum_enabled))
		mptcp_update_data_checksum(skb, copy);
	if (mptcp_subflow_ctx(ssk)->send_infinite_map)
		mptcp_update_infinite_map(msk, ssk, mpext);
	trace_mptcp_sendmsg_frag(mpext);
	mptcp_subflow_ctx(ssk)->rel_write_seq += copy;
}

mptcp_update_data_checksum() queries the missing extension and
unconditionally accesses it. Furthermore, trace_mptcp_sendmsg_frag() lacks
safety guards and will unconditionally dereference the NULL mpext pointer.

[Severity: High]
Can a concurrent transition to fallback mode leave the SKB extension
uninitialized, leading to an information leak?

The MPTCP_FALLBACK_DONE bit can be set locklessly by __mptcp_try_fallback()
while mptcp_sendmsg_frag() is running.

If this concurrent fallback occurs after mptcp_alloc_tx_skb() (which
allocates the uninitialized extension from skbuff_ext_cache) but before the
__mptcp_check_fallback() check, the code takes the goto out branch.

This skips the memset(mpext, 0, ...) initialization that normally happens
later. trace_mptcp_sendmsg_frag() will then dump the uninitialized memory.
If the uninitialized mpext->infinite_map bit happens to be 1,
mptcp_established_options() will mistakenly encode uninitialized garbage
data into a TCP DSS option.

>  	/* 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/70a7e7e05337faa0547c3759e5d9829763f2bcc5.1788244452.git.tanggeliang@kylinos.cn?part=1