Re: [PATCH net 1/5] mptcp: avoid combining some incoming suboptions

[Matthieu Baerts <matttbe@kernel.org>]

Hello,

TL;DR: a v2 is expected

pw-bot: cr

I'm replying here to Clashiko's comments:

> mptcp: avoid combining some incoming suboptions
> 
> This adds combination checks to the incoming MPTCP suboption parser in
> mptcp_parse_option(), reusing the exclusivity table already applied on the
> transmit side, plus a new OPTIONS_MPTCP_DSS helper macro that folds the
> checksum bit into the DSS bit for those checks.
> 
> A few questions about the receive-side behaviour below.
> 
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index c664023d37ba..3b8539f1c224 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -50,6 +50,14 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  			}
>>  		}
>>  
>> +		/* Only the MPC + ACK can be used with a RM_ADDR */
>> +		if (subopt == OPTION_MPTCP_MPC_ACK) {
>> +			if ((mp_opt->suboptions & ~OPTION_MPTCP_RM_ADDR) != 0)
>> +				break;
> 
> Can this drop either the keys or the data mapping when a peer puts an
> MP_CAPABLE(ACK) and a separate DSS in the same third ACK?
> 
> mptcp_get_options() walks the TCP options in wire order and calls
> mptcp_parse_option() once per option, so each gate only sees the options
> that came earlier in the segment:
> 
>   [DSS, MP_CAPABLE ACK] -> this gate sees OPTION_MPTCP_DSS set and breaks,
>                            so sndr_key/rcvr_key are never stored
>   [MP_CAPABLE ACK, DSS] -> the DSS gate below does not allow
>                            OPTIONS_MPTCP_MPC, so the mapping is dropped
> 
> The comment a few lines further down in this same case quotes RFC 8684
> Section 3.1, "the data parameters in a MP_CAPABLE are semantically
> equivalent to those in a DSS option and can be used interchangeably", which
> does not seem to forbid a separate DSS in the same segment.
> 
> Losing the MP_CAPABLE here ends in a fallback to plain TCP; losing the DSS
> leaves the payload of that ACK without a mapping. Neither path bumps a MIB
> counter nor emits a pr_debug, so should there be some counter to make the
> drop visible?

An MPC ACK is not supposed to be used with a DSS: the MPC ACK can carry
a Data-Level Length, and optional Checksum, which are the same as the
ones we can find in a DSS, but they can be present in an MPC ACK because
a DSS is not supposed to be sent in combination to an MPC ACK.

Note that an MPC ACK without data takes 20B, 24B with data. A DSS with
data is taking 20 to 28B, not leaving much space out of the 40B. That's
one reason why they shouldn't be used together.

>> +		} else if (mp_opt->suboptions != 0) {
>> +			break;
>> +		}
> 
> Since the checks are evaluated against the suboptions accumulated so far,
> does the surviving suboption now depend on the order the peer chose, which
> RFC 8684 does not define?
> 
>   [RM_ADDR, MP_CAPABLE SYN] -> this else-if branch breaks, OPTION_MPTCP_MPC_SYN
>                                is never set, subflow_check_req() sees no MPC and
>                                the passive socket stays plain TCP
>   [MP_CAPABLE SYN, RM_ADDR] -> only the RM_ADDR is dropped, MPTCP still works
> 
>   [ADD_ADDR, MP_JOIN SYN]   -> the MP_JOIN gate below tolerates only
>                                RM_ADDR|PRIO, so the join is dropped
>   [MP_JOIN SYN, ADD_ADDR]   -> only the ADD_ADDR is dropped
> 
> Would it be preferable to decide on the full set of parsed suboptions once
> the whole option list has been walked, so that the outcome does not depend
> on sender-side ordering?

I don't think so: having such combinations either means the remote peer
is buggy, or is evil. I don't think the code should try to support these
bad cases, and it should do the simplest action when this is seen: stop
processing the current suboption.

> Do these two gates make the existing MP_CAPABLE + MP_JOIN handling in
> net/mptcp/subflow.c unreachable?
> 
> subflow_check_req() has:
> 
> 	if (opt_mp_capable) {
> 		...
> 		if (opt_mp_join)
> 			return 0;
> 	}
> 
> and mptcp_subflow_init_cookie_req() has:
> 
> 	if (opt_mp_capable && opt_mp_join)
> 		return -EINVAL;

Correct: but it sounds better to keep them here for the moment, just an
extra cheap check. They can be removed in -next in a follow-up patch.

> After this patch mp_opt->suboptions can no longer hold an MPC bit together
> with an MPJ bit, so both branches look dead, and a SYN carrying both
> suboptions is no longer treated as ambiguous but handled as whichever option
> appeared first. Should the commit message mention that semantic change,
> given the Fixes and Cc: stable tags?

Now, all inconsistent combinations are handled the same way. I don't
think we should add an exception to this MPC+MPJ case. A precision could
be added to the commit message for this unexpected error path.

>> +
>>  		/* Cfr RFC 8684 Section 3.3.0:
>>  		 * If a checksum is present but its use had
>>  		 * not been negotiated in the MP_CAPABLE handshake, the receiver MUST
>> @@ -122,6 +130,11 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  		break;
>>  
>>  	case MPTCPOPT_MP_JOIN:
>> +		/* Can be used with a restricted number of other options */
>> +		if ((mp_opt->suboptions & ~(OPTION_MPTCP_RM_ADDR |
>> +					    OPTION_MPTCP_PRIO)) != 0)
>> +			break;
>> +
> 
> Is an MP_JOIN(ACK) combined with a DSS still expected to work?
> 
> MPJ_ACK is 24 bytes and a DSS with a 32-bit mapping is 14 bytes, so 38 bytes
> fit in the TCP option space and both were parsed before this patch. With
> this mask, [DSS, MP_JOIN ACK] drops the MP_JOIN and [MP_JOIN ACK, DSS] drops
> the mapping in the DSS gate below, leaving the payload of the fourth ACK
> without a mapping.

14 bytes is without the "base": Kind + Length + Subtype + Flags. So 4
more bytes are required (+ 2 for the alignment). The total then exceeds 40.

>>  		if (opsize == TCPOLEN_MPTCP_MPJ_SYN) {
>>  			mp_opt->suboptions |= OPTION_MPTCP_MPJ_SYN;
>>  			mp_opt->backup = *ptr++ & MPTCPOPT_BACKUP;
>> @@ -153,6 +166,13 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  		break;
>>  
>>  	case MPTCPOPT_DSS:
>> +		/* Can be used with a restricted number of other options */
>> +		if ((mp_opt->suboptions & ~(OPTION_MPTCP_ADD_ADDR |
>> +					    OPTION_MPTCP_RM_ADDR |
>> +					    OPTION_MPTCP_PRIO |
>> +					    OPTION_MPTCP_FAIL)) != 0)
>> +			break;
>> +
> 
> This isn't a bug introduced by this patch, but it is the same class of
> malformed-combination state the patch is trying to constrain, and the new
> masks cannot see it because they only look at suboption bits.
> 
> In this case the DSS flags are committed to mp_opt before the length is
> validated:
> 
> net/mptcp/options.c:mptcp_parse_option() {
> 	...
> 	flags = (*ptr++) & MPTCP_DSS_FLAG_MASK;
> 	mp_opt->dsn64 = (flags & MPTCP_DSS_DSN64) != 0;
> 	mp_opt->use_map = (flags & MPTCP_DSS_HAS_MAP) != 0;
> 	mp_opt->ack64 = (flags & MPTCP_DSS_ACK64) != 0;
> 	mp_opt->use_ack = (flags & MPTCP_DSS_HAS_ACK);
> 	...
> 	if (opsize != expected_opsize &&
> 	    opsize != expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM)
> 		break;
> 	...
> }
> 
> On that break path no suboption bit is set, so mp_opt->suboptions is still
> 0 and both this gate and the MP_CAPABLE gate above are passed by a second
> option in the same segment, while use_ack/ack64/use_map/data_fin keep the
> values from the rejected DSS.
> 
> mptcp_get_options() only clears the four-byte status group:
> 
> 	*(u32 *)&mp_opt->status = 0;
> 
> so data_ack, data_seq, subflow_seq, data_len and csum of the caller's
> on-stack struct mptcp_options_received are untouched unless the matching
> parse path writes them.
> 
> Two sequences that reach the consumer with those stale flags:
> 
>   [malformed DSS with HAS_ACK, MP_CAPABLE ACK+DATA]
>       MP_CAPABLE sets OPTION_MPTCP_DSS, use_ack is still 1, so
>       mptcp_incoming_options() runs
> 
> 	if (mp_opt.use_ack)
> 		ack_update_msk(msk, sk, &mp_opt);
> 
>       with an uninitialized mp_opt.data_ack feeding mptcp_expand_seq() and
>       the msk->snd_una / msk->wnd_end bookkeeping.
> 
>   [malformed DSS with HAS_MAP|DATA_FIN, valid ack-only DSS]
>       data_fin is only reassigned under if (mp_opt->use_map), so the stale
>       data_fin survives, and on a pure ACK mptcp_incoming_options() runs
> 
> 	if (mp_opt.data_fin && mp_opt.data_len == 1 &&
> 	    mptcp_update_rcv_data_fin(msk, mp_opt.data_seq, mp_opt.dsn64))
> 
>       with uninitialized data_len/data_seq; if the leftover data_len happens
>       to be 1, mptcp_update_rcv_data_fin() records a DATA_FIN at an
>       arbitrary sequence.
> 
> Would it make sense to move the DSS scratch-field assignments after the
> opsize validation, or to zero the whole mp_opt in mptcp_get_options(), so
> the new combination checks operate on a clean state?

Indeed, but that's an existing issue. This will be addressed in a
follow-up patch.

>>  		pr_debug("DSS\n");
>>  		ptr++;
>>  
> 
> [ ... ]
> 
>> @@ -316,6 +357,10 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>>  		break;
>>  
>>  	case MPTCPOPT_MP_FASTCLOSE:
>> +		/* Can be used only with RST */
>> +		if ((mp_opt->suboptions & ~OPTION_MPTCP_RST) != 0)
>> +			break;
>> +
> 
> Does this stop an MP_FASTCLOSE carried in a plain ACK from being honoured?
> 
> RFC 8684 Section 3.5 allows MP_FASTCLOSE in an ACK, and this parser
> deliberately does not require TCPHDR_RST for MP_FASTCLOSE, unlike the
> MPTCPOPT_RST case which does:
> 
> 	if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST))
> 		break;
> 
> An ACK on an established subflow normally also carries a DSS data-ACK, so
> either the FASTCLOSE is dropped here (DSS parsed first) or the DSS is
> dropped by the DSS gate above (FASTCLOSE parsed first, since that mask does
> not include OPTION_MPTCP_FASTCLOSE). When the FASTCLOSE is dropped,
> msk->rcv_fastclose is never set and the connection is not aborted.
> 
> The existing consumer looks like it was written for exactly that
> combination:
> 
> net/mptcp/options.c:mptcp_incoming_options() {
> 	if (unlikely(mp_opt.suboptions != OPTION_MPTCP_DSS)) {
> 		if ((mp_opt.suboptions & OPTION_MPTCP_FASTCLOSE) &&
> 		    READ_ONCE(msk->local_key) == mp_opt.rcvr_key) {
> 			WRITE_ONCE(msk->rcv_fastclose, true);
> 	...
> 		if (!(mp_opt.suboptions & OPTION_MPTCP_DSS))
> 			return true;
> 	}
> 	...
> }
> 
> With this restriction the FASTCLOSE plus DSS fall-through into the DSS
> handling can no longer be reached. Was the intent to make that path dead, or
> should FASTCLOSE be allowed together with OPTIONS_MPTCP_DSS as well?
> 
>>  		if (opsize != TCPOLEN_MPTCP_FASTCLOSE)
>>  			break;
>>  
> 
> [ ... ]

Yes, indeed, in the rcv path, it is possible another networking stack
sends an MP_FASTCLOSE with a DSS, and that's valid.

This will be addressed in v2.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.