Re: [PATCH mptcp-next v8 4/8] mptcp: remove CB offset field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp_inq_hint() can return arbitrarily large, bogus values to userspace (via FIONREAD) due to a missing sequence sync check when the unread data length evaluates to less than INT_MAX, enabling a remotely triggerable Denial of Service.
--

--- Patch [4]: [PATCH mptcp-next v8 4/8] mptcp: remove CB offset field ---
commit f21bf73a134ec25916ac8358db183f0eea8d9925
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: remove CB offset field

This commit introduces a new msk-level field to track the bytes already
consumed inside each skb, similar to TCP, instead of using a CB offset
field. It synchronizes sequence spaces and simplifies hint calculations
and buffer moves.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -2408,26 +2391,26 @@ static bool mptcp_move_skbs(struct sock *sk)
>  	return enqueued;
>  }
>  
> -static unsigned int mptcp_inq_hint(const struct sock *sk)
> +static unsigned int mptcp_inq_hint(struct sock *sk)
>  {
>  	const struct mptcp_sock *msk = mptcp_sk(sk);
> -	const struct sk_buff *skb;
> +	u64 hint_val;
>  
> -	skb = skb_peek(&sk->sk_receive_queue);
> -	if (skb) {
> -		u64 hint_val = atomic64_read(&msk->ack_seq) -
> -			       MPTCP_SKB_CB(skb)->map_seq;
> -
> -		if (hint_val >= INT_MAX)
> -			return INT_MAX;
> -
> -		return (unsigned int)hint_val;
> +	/* Avoid races vs ack_seq updates. */
> +	mptcp_data_lock(sk);
> +	hint_val = atomic64_read(&msk->ack_seq) - msk->copied_seq;
> +	mptcp_data_unlock(sk);
> +	if (hint_val >= INT_MAX) {
> +		if (test_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
> +			return 0;
> +		return INT_MAX;
>  	}

[Severity: High]
Does this logic allow un-synced sequence lengths smaller than INT_MAX to
leak into userspace?

If a server application is actively reading data in recvmsg() (holding the
socket lock) and a remote attacker sends a third ACK with MP_CAPABLE, the
softirq defers updating copied_seq by setting the MPTCP_SYNC_SEQ bit.

Because the test_bit(MPTCP_SYNC_SEQ, ...) check is nested inside the
if (hint_val >= INT_MAX) block, an attacker could bypass the sync check by
crafting a sender_key such that the resulting hint_val is just under INT_MAX.

Could this return an arbitrarily large hint_val to userspace via FIONREAD
(TCP_INQ), potentially leading to massive application-level memory allocations
and rapid memory exhaustion?

>  
> -	if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
> +	if (!hint_val &&
> +	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
>  		return 1;
>  
> -	return 0;
> +	return (unsigned int)hint_val;
>  }
>  
>  static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,

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