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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mptcp: uninitialized more_data_avail and stale seq if
  mptcp_init_skb() fails
--

commit 36fc62d77781e7da07f0272cbd92459df3b0613b
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 sequence numbers for
map_seq and end_seq alongside a 64-bit map_seq64 field so that the shared CB
fields match the TCP control buffer layout.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index cedc91accc6b4..ced34e8cd25c8 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -433,11 +442,13 @@ static bool 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;
> +	MPTCP_SKB_CB(skb)->flags = 0;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;

[Severity: Medium]
In commit "mptcp: remove CB offset field" earlier in this series, can a
failure in mptcp_init_skb() cause __mptcp_move_skbs_from_subflow() to
evaluate more_data_avail uninitialized and leave tp->copied_seq stale?

net/mptcp/protocol.c:mptcp_init_skb() {
	...
	if (unlikely(offset > 0 &&
		     before(TCP_SKB_CB(skb)->seq - subflow->ssn_offset,
			    subflow->map_subflow_seq))) {
		if (offset >= skb->len || !pskb_pull(skb, offset)) {
			WARN_ON_ONCE(1);
			sk_eat_skb(ssk, skb);
			return false;
		}
		offset = 0;
	}
	...
}

net/mptcp/protocol.c:__mptcp_move_skbs_from_subflow() {
	...
	bool more_data_avail;
	...
	do {
		...
		if (offset < skb->len) {
			size_t len = skb->len - offset;

			if (!mptcp_init_skb(ssk, skb, offset))
				continue;
			...
			seq += len;
			...
		}
		...
		WRITE_ONCE(tp->copied_seq, seq);
		more_data_avail = mptcp_subflow_data_available(ssk);

	} while (more_data_avail);
}

When pskb_pull() fails under memory pressure, mptcp_init_skb() consumes
skb via sk_eat_skb() and returns false.  Taking continue in
__mptcp_move_skbs_from_subflow() jumps straight to while (more_data_avail),
reading more_data_avail uninitialized on the first iteration and skipping
both the tp->copied_seq update and mptcp_subflow_data_available().  Because
skb was already removed from ssk->sk_receive_queue while tp->copied_seq was
not advanced, the next skb on the queue computes an underflowed offset
(>= skb->len), dropping it and triggering mptcp_dss_corruption().

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