Re: [PATCH mptcp-next v3 2/3] mptcp: options: track invalid suboptions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: missing invalid option checks in mptcp_parse_option()
--

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

mptcp: options: track invalid suboptions

This commit adds the MPTCP_MIB_OPTINVALID MIB counter to track incoming
MPTCP options that have invalid sizes or unexpected suboption combinations.

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index cbc105199073e..10a150e2ad69e 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -20,7 +20,7 @@ static bool mptcp_cap_flag_sha256(u8 flags)
>  	return (flags & MPTCP_CAP_FLAG_MASK) == MPTCP_CAP_HMAC_SHA256;
>  }
>  
> -static void mptcp_parse_option(const struct sk_buff *skb,
> +static void mptcp_parse_option(const struct sock *sk, const struct sk_buff *skb,
>  			       const unsigned char *ptr, int opsize,
>  			       struct mptcp_options_received *mp_opt)
>  {

[ ... ]

> @@ -175,7 +177,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  					    OPTION_MPTCP_PRIO |
>  					    OPTION_MPTCP_FASTCLOSE |
>  					    OPTION_MPTCP_FAIL)) != 0)
> -			break;
> +			goto invalid;
>  
>  		pr_debug("DSS\n");
>  		ptr++;
> @@ -218,7 +220,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  			mp_opt->ack64 = 0;
>  			mp_opt->use_ack = 0;
>  			mp_opt->data_fin = 0;
> -			break;
> +			goto invalid;
>  		}
>  
>  		mp_opt->suboptions |= OPTION_MPTCP_DSS;

[ ... ]

> @@ -386,10 +388,10 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  		/* Can be used with a restricted number of other options */
>  		if ((mp_opt->suboptions & ~(OPTION_MPTCP_FAIL |
>  					    OPTION_MPTCP_FASTCLOSE)) != 0)
> -			break;
> +			goto invalid;
>  
>  		if (opsize != TCPOLEN_MPTCP_RST)
> -			break;
> +			goto invalid;
>  
>  		if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST))
>  			break;

[ ... ]

> @@ -418,11 +420,17 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  		break;
>  
>  	default:
> +		/* Not invalid: maybe used for experimentations */
>  		break;
>  	}
> +
> +	return;
> +
> +invalid:
> +	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OPTINVALID);
>  }

[Severity: Medium]
Can a couple of invalid option cases in net/mptcp/options.c:mptcp_parse_option()
still read out of bounds or miss incrementing MPTCP_MIB_OPTINVALID?

First, mptcp_get_options() only rejects opsize < 2 before calling
mptcp_parse_option():

net/mptcp/options.c:mptcp_get_options()
	opsize = *ptr++;
	if (opsize < 2) /* "silly options" */
		return;
	if (opsize > length)
		return;	/* don't parse partial options */
	if (opcode == TCPOPT_MPTCP)
		mptcp_parse_option(sk, skb, ptr, opsize,
				   mp_opt);

When opsize == 2, ptr already points past the end of the 2-byte option.
At the start of mptcp_parse_option(), *ptr is dereferenced before checking
opsize:

net/mptcp/options.c:mptcp_parse_option()
	u8 subtype = *ptr >> 4;

If the byte following the 2-byte option has an upper nibble in the default
range (9..15), switch (subtype) hits default: break and returns without
incrementing MPTCP_MIB_OPTINVALID. Similarly, if opsize is 2 or 3 and
subtype is MPTCPOPT_DSS, the MPTCPOPT_DSS case advances ptr and reads a
4th byte before validating opsize against expected_opsize (minimum
TCPOLEN_MPTCP_DSS_BASE == 4):

	case MPTCPOPT_DSS:
		...
		ptr++;

		flags = (*ptr++) & MPTCP_DSS_FLAG_MASK;

Second, in the MPTCPOPT_RST case:

		if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST))
			break;

If a peer sends an MP_RST suboption on a packet without TCPHDR_RST set, it
breaks out instead of jumping to invalid. Aside from not incrementing
MPTCP_MIB_OPTINVALID, this leaves OPTION_MPTCP_RST unset in
mp_opt->suboptions, so a packet carrying MP_RST followed by DSS on a
non-RST segment will accept the DSS option without flagging the invalid
MP_RST + DSS combination.

Should mptcp_parse_option() check for a minimum opsize (and minimum DSS
size before reading flags) and jump to invalid when MPTCPOPT_RST is
received without TCPHDR_RST?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-mptcp-mib-inval-opt-v3-0-428f42a35674@kernel.org?part=2