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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] mptcp: broken skb pull and error path in mptcp_init_skb() and
  caller
--

commit 37ab52009b6bb1043a1c621a00f409a14fa73507
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: remove CB offset field

This commit replaces the per-skb MPTCP control buffer offset field with an
msk-level copied_seq counter and pulls already-consumed prefix bytes when an
skb spans a DSS mapping boundary in mptcp_init_skb().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 74a94264111dc..cedc91accc6b4 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;
> +	}
> +
>  	/* 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;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
>  
>  	__skb_unlink(skb, &ssk->sk_receive_queue);
>  
>  	skb_ext_reset(skb);
>  	skb_dst_drop(skb);
> +
> +	return true;
>  }
[ ... ]
> @@ -855,7 +863,8 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
>  		if (offset < skb->len) {
>  			size_t len = skb->len - offset;
>  
> -			mptcp_init_skb(ssk, skb, offset, len);
> +			if (!mptcp_init_skb(ssk, skb, offset))
> +				continue;
>  
>  			if (own_msk) {
>  				mptcp_subflow_lend_fwdmem(subflow, skb);

[Severity: High]
Does jumping via continue when mptcp_init_skb() returns false evaluate an
uninitialized more_data_avail variable and corrupt tp->copied_seq?

In __mptcp_move_skbs_from_subflow(), more_data_avail is declared without an
initializer and is only assigned at the end of the do-while body:

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 in mptcp_init_skb() (for example, when
__pskb_pull_tail() -> pskb_expand_head(..., GFP_ATOMIC) fails under memory
pressure on a nonlinear or cloned skb), mptcp_init_skb() triggers a
WARN_ON_ONCE(1), calls sk_eat_skb(ssk, skb), and returns false.  On the first
loop iteration, continue jumps straight to while (more_data_avail) and reads
more_data_avail uninitialized.  Because sk_eat_skb() already removed the skb
while continue skips updating tp->copied_seq and calling
mptcp_subflow_data_available(ssk), a subsequent iteration will compute a
wrapped offset = seq - TCP_SKB_CB(skb)->seq >= skb->len on the next skb,
eat it, and trigger a false mptcp_dss_corruption() reset.

In addition, when pskb_pull(skb, offset) succeeds on a nonlinear skb by
expanding its head via pskb_expand_head() and pulling all skb_headlen(skb)
bytes, skb_end_offset(skb) grows without updating skb->truesize while skb->sk
is still ssk.  If that skb is later coalesced as the from skb in
mptcp_try_coalesce() -> skb_try_coalesce(), delta = from->truesize -
SKB_TRUESIZE(skb_end_offset(from)) can become smaller than from->len (or
negative), firing WARN_ON_ONCE(delta < len) and corrupting to->truesize and
sk->sk_rmem_alloc.

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