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

[sashiko-bot@kernel.org]

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