Before this modification, a remote peer could send an MP_CAPABLE with data, with the checksum flag set, but without adding the actual 2 bytes of checksum. As a result, uninitialised bytes could be used for the 'csum' field. That was not a critical issue, because this 'csum' field is only used to compare with the expected one, if previously negotiated in the 3WHS. Worst case, the checksum is likely wrong, a fallback is done without a reject if the negotiation was done earlier. That's OK. Yet, better to take the expected path with this case: only look at the checksum flag for MP_CAPABLEs not carrying a data-len. Such packet can be seen as a 3rd or 4th ACK. The RFC8684 mentions [1] that the 3rd packet should have the checksum flag set. When an MPC + ACK contains data, the checksum flag is redundant with the checksum field. It is not clear what should be done for the 4th ACK, nor if the flag has to be set if the checksum field is set. Therefore, it seems fine to only look at the presence of the checksum field, not to break the interaction with stacks that were not setting both. Note that linked to this checksum flag on the 3rd ACK, with the current implementation, we can have a situation where the SYN packets have no checksum flag, but the 3rd ACK has one, and this is the one that will be taken into account. First, that's clearly not directly linked to this patch, but Clashiko forced us to look at that. At the end, that seems fine to act like that: yes that's not how the negotiation should work, but being flexible without introducing side effects is also fine: fixing this would mean increasing the complexity, and that's not worth it.
Unified · Split
| @@ -93,7 +93,8 @@ static void mptcp_parse_option(const struct sk_buff *skb, · 92 unmodified lines | |||
| 93 | * In other words, the only way for checksums not to be used | 93 | * In other words, the only way for checksums not to be used |
| 94 | * is if both hosts in their SYNs set A=0." | 94 | * is if both hosts in their SYNs set A=0." |
| 95 | */ | 95 | */ |
| 96 | if (flags & MPTCP_CAP_CHECKSUM_REQD) | 96 | if ((flags & MPTCP_CAP_CHECKSUM_REQD) && |
| 97 | opsize < TCPOLEN_MPTCP_MPC_ACK_DATA) | ||
| 97 | mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD; | 98 | mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD; |
| 98 | 99 | ||
| 99 | mp_opt->deny_join_id0 = !!(flags & MPTCP_CAP_DENY_JOIN_ID0); | 100 | mp_opt->deny_join_id0 = !!(flags & MPTCP_CAP_DENY_JOIN_ID0); |