Re: [PATCH mptcp-next v16 3/4] mptcp: sync mptcp skb cb layout with tcp one

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] mptcp: race and copied_seq reset in subflow_set_remote_key() on TFO
- [Critical] mptcp: mptcp_init_skb() subtracts offset from map_seq64 corrupting data
- [High] mptcp: __mptcp_recvmsg_mskq() offset underflow on MSG_PEEK and skb leak
- [Medium] mptcp: mptcp_inq_hint() uses map_seq64 instead of msk->copied_seq
--

commit 67fabb0bf5f4c5ce5c4d525d8c3f0296f5ef7122
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: sync mptcp skb cb layout with tcp one

This commit aligns the MPTCP SKB control buffer layout with the TCP SKB
control buffer by converting map_seq and end_seq to 32-bit fields while
retaining a 64-bit map_seq64 field for sequence validation.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index f89ed94b668d9..bbce1bafa40ee 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -422,11 +429,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]
Can subtracting offset from mptcp_subflow_get_mapped_dsn(subflow) in
mptcp_init_skb() corrupt the MPTCP data stream when an out-of-order DSS
mapping starts at a non-zero offset within a subflow SKB?

In __mptcp_move_skbs_from_subflow(), offset = seq - TCP_SKB_CB(skb)->seq
(where seq is tp->copied_seq) is non-zero when the leading [0, offset)
bytes of the subflow SKB were already consumed on the subflow. For example,
when an SKB spans the end of a previous DSS mapping that was already
acknowledged, subflow_check_data_avail() -> mptcp_subflow_discard_data()
advances tp->copied_seq without eating the SKB and clears
subflow->map_valid, after which get_mapping_status() installs the next
mapping on the remaining [offset, skb->len) bytes:

net/mptcp/subflow.c:subflow_check_data_avail()
	if (unlikely(before64(ack_seq, old_ack))) {
		mptcp_subflow_discard_data(ssk, skb, old_ack - ack_seq);
		continue;
	}

net/mptcp/subflow.c:mptcp_subflow_discard_data()
	if (!before(tcp_sk(ssk)->copied_seq, TCP_SKB_CB(skb)->end_seq))
		sk_eat_skb(ssk, skb);
	if (mptcp_subflow_get_map_offset(subflow) >= subflow->map_data_len)
		subflow->map_valid = 0;

The active DSS mapping only covers skb[offset .. skb->len - 1] starting at
mapped_dsn = mptcp_subflow_get_mapped_dsn(subflow). However,
mptcp_init_skb() sets map_seq64 = mapped_dsn - offset, assigning the MPTCP
sequence range [mapped_dsn - offset, mapped_dsn) to the discarded prefix
bytes skb[0 .. offset - 1].

When mapped_dsn is out of order at the MPTCP level (after64(mapped_dsn,
msk->ack_seq), because the intermediate DSNs were sent on another subflow),
__mptcp_move_skb(), mptcp_data_queue_ofo(), __mptcp_ofo_queue(), and
__mptcp_try_coalesce() treat skb[0 .. offset - 1] as valid payload for
[mapped_dsn - offset, mapped_dsn). Doesn't this advance msk->ack_seq across
that range, deliver the stale subflow prefix bytes to user space, and drop
the actual segment for [mapped_dsn - offset, mapped_dsn) as duplicate data?

> +	MPTCP_SKB_CB(skb)->flags = 0;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
>  	MPTCP_SKB_CB(skb)->cant_coalesce = 0;
>  
> @@ -438,13 +447,13 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
>  
>  static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  {
> -	u64 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
> +	u32 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  	struct sk_buff *tail;
>  
>  	mptcp_borrow_fwdmem(sk, skb);
>  
> -	if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
> +	if (MPTCP_SKB_CB(skb)->map_seq64 == msk->ack_seq) {
>  		/* in sequence */
>  insert:
>  		if (!mptcp_try_rmem_schedule(sk, skb)) {
> @@ -462,14 +471,14 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
>  		skb_set_owner_r(skb, sk);
>  		__skb_queue_tail(&sk->sk_receive_queue, skb);
>  		return true;
> -	} else if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq)) {
> +	} else if (after64(MPTCP_SKB_CB(skb)->map_seq64, msk->ack_seq)) {
>  		mptcp_data_queue_ofo(msk, skb);
>  		return false;
>  	}
>  
>  	/* Partial packet */
> -	if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
> -		copy_len = MPTCP_SKB_CB(skb)->end_seq - msk->ack_seq;
> +	if (after64(MPTCP_SKB_CB(skb)->map_seq64 + skb->len, msk->ack_seq)) {
> +		copy_len = MPTCP_SKB_CB(skb)->end_seq - (u32)msk->ack_seq;
>  		goto insert;
>  	}

[Severity: High]
With MPTCP_SKB_CB(skb)->map_seq now a 32-bit value and msk->copied_seq
tracking read progress, does __mptcp_recvmsg_mskq() underflow offset during
MSG_PEEK and leak fully skipped SKBs on non-peek reads?

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

	skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
		u32 delta, offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
		u32 data_len = skb->len - offset;
		u32 count;
		int err;

		/* Skip skbs that are entirely before copied_seq */
		if (offset >= skb->len) {
			*last = skb;
			continue;
		}

When flags & MSG_PEEK is set, msk->copied_seq is not advanced after copying
from the first SKB in sk->sk_receive_queue. On the second SKB,
MPTCP_SKB_CB(skb)->map_seq is after (u32)msk->copied_seq, so the unsigned
32-bit subtraction (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq wraps
around to a value >= 0x80000000U (>= skb->len). This hits the
offset >= skb->len check and skips every SKB after the first, preventing
MSG_PEEK from ever reading past the first SKB (and causing MSG_PEEK |
MSG_WAITALL to block in sk_wait_data() even when enough data is queued).

Additionally, when !(flags & MSG_PEEK) and an SKB in sk->sk_receive_queue
is genuinely before msk->copied_seq (offset >= skb->len),
__mptcp_recvmsg_mskq() executes *last = skb; continue; without calling
mptcp_eat_recv_skb(sk, skb) (unlike mptcp_recv_skb()). Doesn't that leave
the consumed SKB leaked on sk->sk_receive_queue and charged to
sk->sk_rmem_alloc until the socket is closed?

[ ... ]
> @@ -2392,7 +2399,8 @@ static unsigned int mptcp_inq_hint(const struct sock *sk)
>  
>  	skb = skb_peek(&sk->sk_receive_queue);
>  	if (skb) {
> -		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> +		u64 hint_val = READ_ONCE(msk->ack_seq) -
> +			       MPTCP_SKB_CB(skb)->map_seq64;
>  
>  		if (hint_val >= INT_MAX)
>  			return INT_MAX;

[Severity: Medium]
Should mptcp_inq_hint() subtract msk->copied_seq instead of
MPTCP_SKB_CB(skb)->map_seq64?

When user space partially reads the head SKB in sk->sk_receive_queue via
__mptcp_recvmsg_mskq() or __mptcp_read_sock() (or when a partially
overlapping SKB is queued in __mptcp_move_skb() or __mptcp_ofo_queue()),
msk->copied_seq advances while MPTCP_SKB_CB(skb)->map_seq64 on the head SKB
remains unchanged at its initial sequence number:

net/mptcp/protocol.c:__mptcp_recvmsg_mskq()
		if (!(flags & MSG_PEEK)) {
			msk->bytes_consumed += count;
			msk->copied_seq += count;
			if (count < data_len)
				break;

Computing READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq64 counts the
already-consumed or skipped prefix bytes on the head SKB (including when
subsequent segments coalesce into that head SKB via mptcp_try_coalesce())
as unread.

Beyond reporting inflated unread byte counts for SIOCINQ in mptcp_ioctl()
and TCP_CM_INQ in mptcp_recvmsg(), mptcp_rcv_space_adjust() subtracts
mptcp_inq_hint(sk) from msk->rcvq_space.copied:

net/mptcp/protocol.c:mptcp_rcv_space_adjust()
	copied = msk->rcvq_space.copied;
	copied -= mptcp_inq_hint(sk);
	if (copied <= msk->rcvq_space.space)
		goto new_measure;

Won't subtracting the inflated hint_val cause copied to be under-counted
and prevent mptcp_rcvbuf_grow() from auto-tuning the receive buffer?

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 5e553adb9397e..d144eb2373ef6 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -542,6 +542,7 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
>  		skb = skb_peek(&sk->sk_receive_queue);
>  		if (skb) {
>  			MPTCP_SKB_CB(skb)->map_seq = subflow->iasn - skb->len;
> +			MPTCP_SKB_CB(skb)->map_seq64 = subflow->iasn - skb->len;
>  			MPTCP_SKB_CB(skb)->end_seq = subflow->iasn;
>  			msk->copied_seq -= skb->len;
>  		}

[Severity: Critical]
What happens on passive MPTCP Fast Open if user space accepts the socket
and reads from sk->sk_receive_queue before or while the client's 3rd ACK
triggers subflow_set_remote_key()?

In mptcp_fastopen_subflow_synack_set_params(), the TFO data SKB is queued
onto sk->sk_receive_queue with map_seq64 = -(u64)skb->len and
msk->copied_seq = -(u64)skb->len before the 3rd ACK arrives. Once the child
socket is accepted, user space can call mptcp_recvmsg() ->
__mptcp_recvmsg_mskq() (or __mptcp_read_sock() -> mptcp_recv_skb()) under
lock_sock(sk) without holding mptcp_data_lock(sk), while the 3rd ACK
arrives in softirq via check_fully_established() ->
__mptcp_subflow_fully_established() -> subflow_set_remote_key() (or
mptcp_propagate_state() -> subflow_set_remote_key()) under
mptcp_data_lock(sk) without checking sock_owned_by_user(sk):

net/mptcp/subflow.c:subflow_set_remote_key()
	msk->copied_seq = subflow->iasn;

	if (subflow->is_mptfo) {
		struct sock *sk = (struct sock *)msk;
		struct sk_buff *skb;

		skb = skb_peek(&sk->sk_receive_queue);
		if (skb) {
			MPTCP_SKB_CB(skb)->map_seq = subflow->iasn - skb->len;
			MPTCP_SKB_CB(skb)->map_seq64 = subflow->iasn - skb->len;
			MPTCP_SKB_CB(skb)->end_seq = subflow->iasn;
			msk->copied_seq -= skb->len;
		}
	}

This appears to cause three regressions:

1. If user space partially reads count bytes (0 < count < skb->len) from
the TFO SKB before the 3rd ACK arrives, __mptcp_recvmsg_mskq() advances
msk->copied_seq to -(u64)skb->len + count while leaving the SKB (with
skb->len unchanged) at the head of sk->sk_receive_queue. When
subflow_set_remote_key() runs, it sets msk->copied_seq to
subflow->iasn - skb->len, discarding the partial read progress so the next
recvmsg() re-delivers the first count bytes.

2. In __mptcp_recvmsg_mskq() and mptcp_recv_skb(), mptcp_eat_recv_skb()
unlinks the TFO SKB from sk->sk_receive_queue and frees it via
skb_attempt_defer_free(skb) under lock_sock(sk) without mptcp_data_lock(sk).
If mptcp_eat_recv_skb() unlinks and frees skb right after
subflow_set_remote_key() peeks it with skb_peek(&sk->sk_receive_queue),
can't subflow_set_remote_key() perform a use-after-free write to
MPTCP_SKB_CB(skb)?

3. In __mptcp_recvmsg_mskq(), msk->copied_seq += count runs before
mptcp_eat_recv_skb(sk, skb). If subflow_set_remote_key() executes between
those two lines when the TFO SKB is completely read, skb_peek() still sees
the SKB and sets msk->copied_seq = subflow->iasn - skb->len before
mptcp_eat_recv_skb() unlinks it, leaving msk->copied_seq permanently behind
msk->ack_seq by skb->len and causing subsequent SKBs to underflow offset
and be skipped.

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