Re: [PATCH mptcp-next v21 8/9] mptcp: sync mptcp skb cb layout with tcp one

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mptcp: skb_pull() BUG() on nonlinear skb and bad offset in mptcp_init_skb()
- [High] mptcp: wrap-unsafe mapping boundary check in mptcp_init_skb()
--

commit 9d35387c93b4a664b7d58d9f115a8985e1df6730
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: sync mptcp skb cb layout with tcp one

This commit updates struct mptcp_skb_cb to use 32-bit map_seq and end_seq
fields aligned with struct tcp_skb_cb, while adding a 64-bit map_seq64 field
for window validation and coalescing checks.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0666bbfdf2cb8..705b28219883f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -437,11 +446,13 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
>  
>  	/* the skb map_seq accounts for the skb offset:
>  	 * mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
> -	 * value
> +	 * value; note that end seq number is only available in 32bits format.
>  	 */
> -	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
> -				     offset;
> +	MPTCP_SKB_CB(skb)->map_seq64 = mptcp_subflow_get_mapped_dsn(subflow) -
> +				       offset;
> +	MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
>  	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;

[Severity: Critical]
As reported by Sashiko on v20 [1], in mptcp_init_skb():

	/* Detect mapping boundary: skb spans multiple mappings */
	if (unlikely(offset > 0 &&
		     TCP_SKB_CB(skb)->seq - subflow->ssn_offset <
		     subflow->map_subflow_seq)) {
		...
		WARN_ON_ONCE(offset >= skb->len);
		skb_pull(skb, offset);
		offset = 0;
	}

	MPTCP_SKB_CB(skb)->map_seq64 = mptcp_subflow_get_mapped_dsn(subflow) -
				       offset;
	MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;

can calling skb_pull(skb, offset) here trigger a BUG() in __skb_pull() when
the incoming subflow skb is non-linear?

In tcp_data_queue(), the TCP header has already been pulled via __skb_pull().
When the TCP payload resides in page fragments (for example, skbs generated by
mptcp_sendmsg_frag() where only MAX_TCP_HEADER linear space is allocated, GRO
packets, page_pool buffers, or segments coalesced by tcp_try_coalesce()),
skb_headlen(skb) can be 0 or smaller than offset. Because skb_pull() calls
__skb_pull():

	skb->len -= len;
	if (unlikely(skb->len < skb->data_len)) {
		...
		BUG();
	}

pulling offset without pskb_pull() or __pskb_trim_head() reduces skb->len
below skb->data_len and panics the kernel whenever offset > skb_headlen(skb).

Additionally, when offset > 0 within a single DSS mapping (so the mapping
boundary branch is not taken), mptcp_init_skb() leaves the [0, offset) prefix
attached and subtracts offset from map_seq64. Since validate_data_csum() only
validates data starting at tp->copied_seq, if this skb is queued out of order
in __mptcp_move_skb() -> mptcp_data_queue_ofo():

		if (before(seq, MPTCP_SKB_CB(skb1)->end_seq)) {
			if (!after((u32)end_seq, MPTCP_SKB_CB(skb1)->end_seq)) {
				...
			}
			if (after(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
				...
			} else {
				rb_replace_node(&skb1->rbnode, &skb->rbnode,
						&msk->out_of_order_queue);
				mptcp_drop(sk, skb1);
				MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
				goto merge_right;
			}
		}

won't the offset-shifted map_seq cause mptcp_data_queue_ofo() to replace an
already-validated skb1 at mapped_dsn - offset with the unvalidated [0, offset)
prefix, or prevent mptcp_try_coalesce() when skb1 starts earlier?

[Severity: High]
Also in mptcp_init_skb(), the mapping boundary check compares 32-bit relative
subflow sequence numbers using unsigned <:

	if (unlikely(offset > 0 &&
		     TCP_SKB_CB(skb)->seq - subflow->ssn_offset <
		     subflow->map_subflow_seq)) {

Unlike validate_mapping() in net/mptcp/subflow.c, which uses before():

	u32 ssn = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;

	if (unlikely(before(ssn, subflow->map_subflow_seq))) {

what happens when the 32-bit relative subflow sequence number wraps around
2^32 (or when TCP_SKB_CB(skb)->seq precedes subflow->ssn_offset, such as a
segment overlapping TFO SYN data)?

If the skb starts before the wrap in an old mapping (for example,
TCP_SKB_CB(skb)->seq - subflow->ssn_offset == 0xfffffe00) and the current
mapping starts after the wrap (for example, subflow->map_subflow_seq ==
0x00000100), unsigned < evaluates to false. Wouldn't mptcp_init_skb() then
skip removing the old mapping's prefix bytes and subtract offset from
mptcp_subflow_get_mapped_dsn(subflow), attributing stale prefix bytes from
the old mapping to the new mapping's DSN range [mapped_dsn - offset,
mapped_dsn)? Conversely, if a single mapping straddles the 2^32 wrap boundary,
wouldn't an skb starting after the wrap with offset > 0 falsely satisfy
unsigned < and strip valid prefix bytes?

[1] https://lore.kernel.org/r/cf2994ec4440d24183bea5c407c1962270392c32.1793695518.git.tanggeliang@kylinos.cn

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793870710.git.tanggeliang@kylinos.cn?part=8