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

[Geliang Tang <geliang@kernel.org>]

On Thu, 2026-10-29 at 21:52 +0000, sashiko-bot@kernel.org wrote:
> 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;

These could be addressed by adding an "opsize < 3" check at the
beginning of mptcp_parse_option(). However, this issue wasn't
introduced by this patch - I think they can be ignored for now and
addressed in a separate patch if needed.

> 
> Second, in the MPTCPOPT_RST case:
> 
> 		if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST))
> 			break;

Indeed, this should use goto invalid; instead of break. WDYT?

Thanks,
-Geliang

> 
> 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?