Re: [PATCH mptcp-next 3/3] mptcp: options: reset parsing in case of invalidity

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `break` statement inside `mptcp_get_options` incorrectly exits the `switch` statement instead of the `while` loop, causing `length` and `ptr` to become desynchronized, leading to an infinite loop, out-of-bounds reads, and MPTCP option smuggling.
--

commit 60e4bc5aa7cfc08049c3062323eeae263a824439
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: options: reset parsing in case of invalidity

When an invalid option is detected, it is safer to drop any previously
parsed MPTCP options. This ensures a fallback to TCP or a subflow reject
instead of just ignoring the currently parsed option.

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index aaaabcab0f8f9..b6d2e39f8464a 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
[ ... ]
> @@ -461,9 +456,12 @@ void mptcp_get_options(const struct sock *sk, const struct sk_buff *skb,
>  				return;
>  			if (opsize > length)
>  				return;	/* don't parse partial options */
> -			if (opcode == TCPOPT_MPTCP)
> -				mptcp_parse_option(sk, skb, ptr, opsize,
> -						   mp_opt);
> +			if (opcode == TCPOPT_MPTCP &&
> +			    !mptcp_parse_option(sk, skb, ptr, opsize, mp_opt)) {
> +				/* invalid option, drop previously parsed opt */
> +				*(u32 *)&mp_opt->status = 0;
> +				break;

[Severity: High]
Does this break statement exit the switch rather than the while loop?

If this only exits the switch block, it will skip the pointer and length
updates that occur at the end of the default case:

    ptr += opsize - 2;
    length -= opsize;

Since length is not decremented when breaking here, the while (length > 0)
loop will continue indefinitely. The ptr keeps advancing by 2 bytes on each
subsequent iteration from reading opcode and opsize. Could this lead to an
infinite loop and out-of-bounds reads into the TCP payload?

Additionally, could this allow an attacker to smuggle MPTCP options by
placing them in the TCP payload after an invalid option that triggers this
break? Because length is never updated, the parser might treat the TCP payload
as an extension of the TCP options, bypassing inspection that only checks the
TCP header.

> +			}
>  			ptr += opsize - 2;
>  			length -= opsize;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-mptcp-mib-inval-opt-v1-0-84a553c552b6@kernel.org?part=3