Re: [PATCH mptcp-next v20 5/6] 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: subtracting offset in mptcp_init_skb() corrupts out-of-order data
- [High] mptcp: 64-bit ack_seq minus 32-bit map_seq overflow in mptcp_inq_hint()
--

commit 0495d49fc9571bbbbcc61fc3640dabeb8f98b52b
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: sync mptcp skb cb layout with tcp one

This commit aligns struct mptcp_skb_cb with struct tcp_skb_cb by narrowing
map_seq and end_seq to 32-bit sequence numbers and adding a 64-bit
map_seq64 field for receive window admission and coalescing checks.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3bd7d38ce78c4..3e10b06b7772d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -425,10 +434,14 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
>  	 *
>  	 * This is correct even when offset > 0 and mapping is out-of-order,
>  	 * because consumed prefix bytes are part of the same logical DSN range.
> +	 *
> +	 * 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;
> +	MPTCP_SKB_CB(skb)->flags = 0;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;

[Severity: Critical]
As reported by Sashiko on v19 [1], when offset > 0 in mptcp_init_skb() and
the active DSS mapping is out of order at the MPTCP level
(mptcp_subflow_get_mapped_dsn(subflow) > msk->ack_seq), can subtracting
offset from map_seq64 and map_seq without stripping the [0, offset) prefix
from skb corrupt the MPTCP receive stream?

On a subflow where data is multiplexed across multiple subflows,
consecutive DSS mappings do not have contiguous DSNs. If the leading
[0, offset) bytes of the head skb on ssk->sk_receive_queue belonged to a
previous DSS mapping (for example after mptcp_subflow_discard_data() in
subflow_check_data_avail() discards an already-received mapping and
advances to the next mapping in the same skb) or an overlapping TCP
retransmission, those prefix bytes are not part of the active mapping
starting at tp->copied_seq and are not covered by its DSS checksum in
validate_data_csum().

Setting map_seq64 = mapped_dsn - offset and end_seq = map_seq + skb->len
assigns the fabricated DSN range [mapped_dsn - offset, mapped_dsn) to the
unpulled [0, offset) prefix bytes:

- If mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb()
  treats the skb as covering msk->ack_seq, queues or coalesces it into
  sk->sk_receive_queue, and advances msk->ack_seq across the unreceived
  gap. __mptcp_recvmsg_mskq() and mptcp_recv_skb() then compute a read
  offset of (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq < offset and
  copy the stale [0, offset) subflow bytes to user space.

- If mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts the
  skb into msk->out_of_order_queue starting at mapped_dsn - offset, where
  __mptcp_try_coalesce() can coalesce the stale prefix into a preceding OFO
  skb, real segments arriving for [mapped_dsn - offset, mapped_dsn) are
  dropped as duplicate data, and __mptcp_ofo_queue() dequeues the skb once
  msk->ack_seq reaches mapped_dsn - offset.

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 7878e6ac6c0b6..fa54f70ab501d 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -129,9 +129,12 @@
>  #define MPTCP_SYNC_SEQ		8
>  
>  struct mptcp_skb_cb {
> -	u64 map_seq;
> -	u64 end_seq;
> +	u32 map_seq;
> +	u32 end_seq;
> +	u32 unused;
> +	u16 flags;
>  	u8  has_rxtstamp;
> +	u64 map_seq64;
>  };

[Severity: High]
Now that MPTCP_SKB_CB(skb)->map_seq is a u32, does mptcp_inq_hint()
overflow when subtracting it from the 64-bit msk->ack_seq?

In net/mptcp/protocol.c:mptcp_inq_hint():

	skb = skb_peek(&sk->sk_receive_queue);
	if (skb) {
		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;

		if (hint_val >= INT_MAX)
			return INT_MAX;

		return (unsigned int)hint_val;
	}

Because READ_ONCE(msk->ack_seq) is a u64 and MPTCP_SKB_CB(skb)->map_seq is
zero-extended from u32 to u64, the upper 32 bits of msk->ack_seq remain in
hint_val (or the subtraction underflows across a 32-bit wrap), causing
hint_val >= INT_MAX to evaluate to true whenever sk->sk_receive_queue is
non-empty and msk->ack_seq has non-zero upper 32 bits.

In addition to returning INT_MAX for SIOCINQ in mptcp_ioctl() and
TCP_CM_INQ in mptcp_recvmsg(), mptcp_rcv_space_adjust() does:

	copied = msk->rcvq_space.copied;
	copied -= mptcp_inq_hint(sk);
	if (copied <= msk->rcvq_space.space)
		goto new_measure;

When mptcp_inq_hint() returns INT_MAX, copied becomes negative and jumps to
new_measure, preventing mptcp_rcvbuf_grow() from auto-tuning the MPTCP and
subflow receive buffers. Should mptcp_inq_hint() use 32-bit sequence
arithmetic (or subtract msk->copied_seq from msk->ack_seq) instead?

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

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