Re: [PATCH mptcp-next v23 6/8] mptcp: remove CB offset field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] mptcp: truesize corruption after pskb_pull() in
  mptcp_init_skb()
- [Medium] mptcp: unpulled skb prefix in mptcp_init_skb() rewinds
  map_seq
--

commit 6d46cc996583689225db9776f596454406aeafdb
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: remove CB offset field

This commit replaces the per-skb MPTCP_SKB_CB(skb)->offset field with an
msk-level copied_seq counter to track consumed receive data across skbs.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index a6c4bad0a8b96..f75297c4d6b0d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -415,25 +414,38 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  	skb_set_owner_r(skb, sk);
>  }
>  
> -static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
> -			   int copy_len)
> +static bool mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>  	bool has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
>  
> +	/* Detect mapping boundary: skb spans multiple mappings */
> +	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;
> +	}

[Severity: High]
Can calling pskb_pull() here corrupt skb->truesize and socket memory
accounting when skb is nonlinear?

At this point skb is still owned by ssk with skb->destructor set to
sock_rfree.  When offset exceeds skb_headlen(skb) and the linear head
lacks tailroom (or skb is cloned), __pskb_pull_tail() calls
pskb_expand_head(), which increases skb_end_offset(skb) without updating
skb->truesize because skb->sk is set:

net/core/skbuff.c:pskb_expand_head() {
	...
	if (!skb->sk || skb->destructor == sock_edemux)
		skb->truesize += size - osize;
}

pskb_pull() then advances skb->data to skb->tail, leaving
skb_headlen(skb) == 0.  If this skb is later coalesced as the from
buffer in __mptcp_try_coalesce() -> skb_try_coalesce(), the
skb_headlen(from) == 0 path computes:

	delta = from->truesize - SKB_TRUESIZE(skb_end_offset(from));

Because skb_end_offset(from) grew while from->truesize did not, delta is
underestimated by the head expansion size and can be smaller than
from->len or negative, triggering WARN_ON_ONCE(delta < len) in
skb_try_coalesce() and corrupting to->truesize and sk_rmem_alloc.

> +
>  	/* the skb map_seq accounts for the skb offset:
>  	 * mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
>  	 * value
>  	 */
> -	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow);
> -	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
> -	MPTCP_SKB_CB(skb)->offset = offset;
> +	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
> +				     offset;
> +	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;

[Severity: Medium]
When a TCP subflow queues a partially overlapping segment that starts
within the current DSS mapping, offset is positive while
before(TCP_SKB_CB(skb)->seq - subflow->ssn_offset,
subflow->map_subflow_seq) is false.  Does rewinding map_seq by offset
without pulling the prefix expose unvalidated bytes to the MPTCP queues?

When data checksums are enabled, validate_data_csum() starts at
tcp_sk(ssk)->copied_seq + subflow->map_csum_len, so the already-consumed
prefix skb[0..offset) in an overlapping segment was never checked
against the DSS checksum.  Rewinding map_seq to include that prefix
causes mptcp_data_queue_ofo() to treat the unpulled bytes as valid
payload covering earlier DSNs, replacing and dropping previously
validated out-of-order skbs:

net/mptcp/protocol.c:mptcp_data_queue_ofo() {
	...
			} else {
				/* skb's seq == skb1's seq and skb covers skb1.
				 * Replace skb1 with skb.
				 */
				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;
			}
}

When msk->ack_seq later advances to that mapping, userspace reads the
unvalidated prefix from skb instead of the validated bytes that were in
skb1.

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