[PATCH mptcp-next v3 0/3] mptcp: options: MIB for invalid & drop suboptions

[PATCH mptcp-next v3 0/3] mptcp: options: MIB for invalid & drop suboptions

[Matthieu Baerts (NGI0)]

Here are a few improvements linked to the parsing of the MPTCP options:

- Patch 1: Move the only non-suboptions from the 'suboptions' field.

- Patch 2: Track invalid suboptions: unexpected suboptions combinations
  or invalid sizes.

- Patch 3: drop any previously parsed suboptions in case of invalidity.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Changes in v3:
- No changes, just a rebase with a simple conflict resolution.
- Link to v2: https://patch.msgid.link/20260904-mptcp-mib-inval-opt-v2-0-a72003dc4425@kernel.org

Changes in v2:
- Patch 3: break the while-loop, not the switch-case.
- Link to v1: https://patch.msgid.link/20260903-mptcp-mib-inval-opt-v1-0-84a553c552b6@kernel.org

---
Matthieu Baerts (NGI0) (3):
      mptcp: options: use a dedicated bit for csum reqd
      mptcp: options: track invalid suboptions
      mptcp: options: reset parsing in case of invalidity

 net/mptcp/mib.c      |  1 +
 net/mptcp/mib.h      |  1 +
 net/mptcp/options.c  | 94 ++++++++++++++++++++++++++++------------------------
 net/mptcp/protocol.c |  2 +-
 net/mptcp/protocol.h |  8 ++---
 net/mptcp/subflow.c  | 12 +++----
 6 files changed, 62 insertions(+), 56 deletions(-)
---
base-commit: bcc5638fabf418e763621a432840cf55287e604d
change-id: 20260729-mptcp-mib-inval-opt-ac1605020170

Best regards,
--  
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-next v3 1/3] mptcp: options: use a dedicated bit for csum reqd

[Matthieu Baerts (NGI0)]

Instead of mixing that with suboptions: that's the only non-suboptions
that is present in the 'suboptions' field.

This even cause a workaround with OPTIONS_MPTCP_DSS to exclude this
non-suboptions when checking which other ones are set.

Move it to a dedicated free bit, and adapt the corresponding code.

This is clearer like that.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c  | 25 ++++++++++++-------------
 net/mptcp/protocol.c |  2 +-
 net/mptcp/protocol.h |  6 ++----
 net/mptcp/subflow.c  |  2 +-
 4 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index adbf8e0773db..cbc105199073 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -93,9 +93,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		 * In other words, the only way for checksums not to be used
 		 * is if both hosts in their SYNs set A=0."
 		 */
-		if ((flags & MPTCP_CAP_CHECKSUM_REQD) &&
-		    opsize < TCPOLEN_MPTCP_MPC_ACK_DATA)
-			mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
+		mp_opt->csum_reqd = (flags & MPTCP_CAP_CHECKSUM_REQD) &&
+				    opsize < TCPOLEN_MPTCP_MPC_ACK_DATA;
 
 		mp_opt->deny_join_id0 = !!(flags & MPTCP_CAP_DENY_JOIN_ID0);
 
@@ -122,7 +121,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		}
 		if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM) {
 			mp_opt->csum = get_unaligned((__force __sum16 *)ptr);
-			mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
+			mp_opt->csum_reqd = 1;
 			ptr += 2;
 		}
 		pr_debug("MP_CAPABLE version=%x, flags=%x, optlen=%d sndr=%llu, rcvr=%llu len=%d csum=%u\n",
@@ -251,22 +250,22 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 			ptr += 2;
 
 			if (opsize == expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM) {
-				mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
 				mp_opt->csum = get_unaligned((__force __sum16 *)ptr);
+				mp_opt->csum_reqd = 1;
 				ptr += 2;
 			}
 
 			pr_debug("data_seq=%llu subflow_seq=%u data_len=%u csum=%d:%u\n",
 				 mp_opt->data_seq, mp_opt->subflow_seq,
-				 mp_opt->data_len, !!(mp_opt->suboptions & OPTION_MPTCP_CSUMREQD),
-				 (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD) ? mp_opt->csum : 0);
+				 mp_opt->data_len, mp_opt->csum_reqd,
+				 mp_opt->csum_reqd ? mp_opt->csum : 0);
 		}
 
 		break;
 
 	case MPTCPOPT_ADD_ADDR:
 		/* Can be used with a restricted number of other options */
-		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_DSS |
+		if ((mp_opt->suboptions & ~(OPTION_MPTCP_DSS |
 					    OPTION_MPTCP_RM_ADDR |
 					    OPTION_MPTCP_PRIO)) != 0)
 			break;
@@ -333,7 +332,7 @@ 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_MPC_ACK |
 					    OPTIONS_MPTCP_MPJ |
-					    OPTIONS_MPTCP_DSS |
+					    OPTION_MPTCP_DSS |
 					    OPTION_MPTCP_ADD_ADDR |
 					    OPTION_MPTCP_PRIO)) != 0)
 			break;
@@ -354,7 +353,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 	case MPTCPOPT_MP_PRIO:
 		/* Can be used with a restricted number of other options */
 		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_MPJ |
-					    OPTIONS_MPTCP_DSS |
+					    OPTION_MPTCP_DSS |
 					    OPTION_MPTCP_ADD_ADDR |
 					    OPTION_MPTCP_RM_ADDR)) != 0)
 			break;
@@ -369,7 +368,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 
 	case MPTCPOPT_MP_FASTCLOSE:
 		/* Can be used with a restricted number of other options */
-		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_DSS |
+		if ((mp_opt->suboptions & ~(OPTION_MPTCP_DSS |
 					    OPTION_MPTCP_RST)) != 0)
 			break;
 
@@ -405,7 +404,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 
 	case MPTCPOPT_MP_FAIL:
 		/* Can be used with a restricted number of other options */
-		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_DSS |
+		if ((mp_opt->suboptions & ~(OPTION_MPTCP_DSS |
 					    OPTION_MPTCP_RST)) != 0)
 			break;
 
@@ -1352,7 +1351,7 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 		}
 		mpext->data_len = mp_opt.data_len;
 		mpext->use_map = 1;
-		mpext->csum_reqd = !!(mp_opt.suboptions & OPTION_MPTCP_CSUMREQD);
+		mpext->csum_reqd = mp_opt.csum_reqd;
 
 		if (mpext->csum_reqd)
 			mpext->csum = mp_opt.csum;
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 02a4f479d9d8..c85401952bf1 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3798,7 +3798,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
 	WRITE_ONCE(msk->token, subflow_req->token);
 	msk->in_accept_queue = 1;
 	WRITE_ONCE(msk->fully_established, false);
-	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
+	if (mp_opt->csum_reqd)
 		WRITE_ONCE(msk->csum_enabled, true);
 
 	WRITE_ONCE(msk->write_seq, subflow_req->idsn + 1);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3be60da12924..cb1862ca5cb3 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -31,13 +31,10 @@
 #define OPTION_MPTCP_DSS	BIT(11)
 #define OPTION_MPTCP_FAIL	BIT(12)
 
-#define OPTION_MPTCP_CSUMREQD	BIT(13)
-
 #define OPTIONS_MPTCP_MPC	(OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK | \
 				 OPTION_MPTCP_MPC_ACK)
 #define OPTIONS_MPTCP_MPJ	(OPTION_MPTCP_MPJ_SYN | OPTION_MPTCP_MPJ_SYNACK | \
 				 OPTION_MPTCP_MPJ_ACK)
-#define OPTIONS_MPTCP_DSS	(OPTION_MPTCP_DSS | OPTION_MPTCP_CSUMREQD)
 
 /* MPTCP option subtypes */
 #define MPTCPOPT_MP_CAPABLE	0
@@ -165,7 +162,8 @@ struct mptcp_options_received {
 		    echo:1,
 		    backup:1,
 		    deny_join_id0:1,
-		    __unused:2;
+		    csum_reqd:1,
+		    __unused:1;
 	);
 	struct mptcp_rm_list rm_list;
 	/* Options below are mutually exclusive, see mptcp_parse_option() */
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 78e886b20258..b1aa772a2519 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -592,7 +592,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 			goto fallback;
 		}
 
-		if (mp_opt.suboptions & OPTION_MPTCP_CSUMREQD)
+		if (mp_opt.csum_reqd)
 			WRITE_ONCE(msk->csum_enabled, true);
 		if (mp_opt.deny_join_id0)
 			WRITE_ONCE(msk->pm.remote_deny_join_id0, true);

-- 
2.55.0

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

[Matthieu Baerts (NGI0)]

Any unexpected suboptions combinations or use of invalid sizes are
suspicious: either it is coming from a buggy host, or an attacker.

Better to track that with a new MIB counter.

Note that receiving unsupported versions or extensions can happen, so
this part shouldn't be considered as invalid.

Co-developed-by: Gang Yan <yangang@kylinos.cn>
Signed-off-by: Gang Yan <yangang@kylinos.cn>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/mib.c      |  1 +
 net/mptcp/mib.h      |  1 +
 net/mptcp/options.c  | 55 ++++++++++++++++++++++++++++++----------------------
 net/mptcp/protocol.h |  2 +-
 net/mptcp/subflow.c  | 10 +++++-----
 5 files changed, 40 insertions(+), 29 deletions(-)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index 608cb568897c..c54a20f19cf9 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -95,6 +95,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("BacklogDrop", MPTCP_MIB_BACKLOGDROP),
 	SNMP_MIB_ITEM("RcvPruned", MPTCP_MIB_RCVPRUNED),
 	SNMP_MIB_ITEM("OFOPruned", MPTCP_MIB_OFOPRUNED),
+	SNMP_MIB_ITEM("OptInvalid", MPTCP_MIB_OPTINVALID),
 };
 
 /* mptcp_mib_alloc - allocate percpu mib counters
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 1ebdb55e9534..d1ce731cb5b7 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -98,6 +98,7 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_BACKLOGDROP,		/* Backlog over memory limit */
 	MPTCP_MIB_RCVPRUNED,		/* Dropped due to memory constraints */
 	MPTCP_MIB_OFOPRUNED,		/* MPTCP-level OoO queue pruned */
+	MPTCP_MIB_OPTINVALID,		/* Invalid MPTCP option */
 	__MPTCP_MIB_MAX
 };
 
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index cbc105199073..10a150e2ad69 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)
 {
@@ -53,9 +53,9 @@ 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;
+				goto invalid;
 		} else if (mp_opt->suboptions != 0) {
-			break;
+			goto invalid;
 		}
 
 		/* Cfr RFC 8684 Section 3.3.0:
@@ -71,7 +71,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		if (opsize != expected_opsize &&
 		    (expected_opsize != TCPOLEN_MPTCP_MPC_ACK_DATA ||
 		     opsize != TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM))
-			break;
+			goto invalid;
 
 		/* try to be gentle vs future versions on the initial syn */
 		version = *ptr++ & MPTCP_VERSION_MASK;
@@ -136,7 +136,7 @@ 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_RM_ADDR |
 					    OPTION_MPTCP_PRIO)) != 0)
-			break;
+			goto invalid;
 
 		if (opsize == TCPOLEN_MPTCP_MPJ_SYN) {
 			mp_opt->suboptions |= OPTION_MPTCP_MPJ_SYN;
@@ -165,6 +165,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 			ptr += 2;
 			memcpy(mp_opt->hmac, ptr, MPTCPOPT_HMAC_LEN);
 			pr_debug("MP_JOIN hmac\n");
+		} else {
+			goto invalid;
 		}
 		break;
 
@@ -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;
@@ -268,7 +270,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		if ((mp_opt->suboptions & ~(OPTION_MPTCP_DSS |
 					    OPTION_MPTCP_RM_ADDR |
 					    OPTION_MPTCP_PRIO)) != 0)
-			break;
+			goto invalid;
 
 		mp_opt->echo = (*ptr++) & MPTCP_ADDR_ECHO;
 		if (!mp_opt->echo) {
@@ -281,7 +283,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 				mp_opt->addr.family = AF_INET6;
 #endif
 			else
-				break;
+				goto invalid;
 		} else {
 			if (opsize == TCPOLEN_MPTCP_ADD_ADDR_BASE ||
 			    opsize == TCPOLEN_MPTCP_ADD_ADDR_BASE_PORT)
@@ -292,7 +294,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 				mp_opt->addr.family = AF_INET6;
 #endif
 			else
-				break;
+				goto invalid;
 		}
 
 		mp_opt->suboptions |= OPTION_MPTCP_ADD_ADDR;
@@ -335,11 +337,11 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 					    OPTION_MPTCP_DSS |
 					    OPTION_MPTCP_ADD_ADDR |
 					    OPTION_MPTCP_PRIO)) != 0)
-			break;
+			goto invalid;
 
 		if (opsize < TCPOLEN_MPTCP_RM_ADDR_BASE + 1 ||
 		    opsize > TCPOLEN_MPTCP_RM_ADDR_BASE + MPTCP_RM_IDS_MAX)
-			break;
+			goto invalid;
 
 		ptr++;
 
@@ -356,10 +358,10 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 					    OPTION_MPTCP_DSS |
 					    OPTION_MPTCP_ADD_ADDR |
 					    OPTION_MPTCP_RM_ADDR)) != 0)
-			break;
+			goto invalid;
 
 		if (opsize != TCPOLEN_MPTCP_PRIO)
-			break;
+			goto invalid;
 
 		mp_opt->suboptions |= OPTION_MPTCP_PRIO;
 		mp_opt->backup = *ptr++ & MPTCP_PRIO_BKUP;
@@ -370,10 +372,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_DSS |
 					    OPTION_MPTCP_RST)) != 0)
-			break;
+			goto invalid;
 
 		if (opsize != TCPOLEN_MPTCP_FASTCLOSE)
-			break;
+			goto invalid;
 
 		ptr += 2;
 		mp_opt->fc_recv_key = get_unaligned_be64(ptr);
@@ -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;
@@ -406,10 +408,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_DSS |
 					    OPTION_MPTCP_RST)) != 0)
-			break;
+			goto invalid;
 
 		if (opsize != TCPOLEN_MPTCP_FAIL)
-			break;
+			goto invalid;
 
 		ptr += 2;
 		mp_opt->suboptions |= OPTION_MPTCP_FAIL;
@@ -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);
 }
 
-void mptcp_get_options(const struct sk_buff *skb,
+void mptcp_get_options(const struct sock *sk, const struct sk_buff *skb,
 		       struct mptcp_options_received *mp_opt)
 {
 	const struct tcphdr *th = tcp_hdr(skb);
@@ -457,7 +465,8 @@ void mptcp_get_options(const struct sk_buff *skb,
 			if (opsize > length)
 				return;	/* don't parse partial options */
 			if (opcode == TCPOPT_MPTCP)
-				mptcp_parse_option(skb, ptr, opsize, mp_opt);
+				mptcp_parse_option(sk, skb, ptr, opsize,
+						   mp_opt);
 			ptr += opsize - 2;
 			length -= opsize;
 		}
@@ -1251,7 +1260,7 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 		return !mptcp_over_limit(subflow->conn, sk, skb);
 	}
 
-	mptcp_get_options(skb, &mp_opt);
+	mptcp_get_options(sk, skb, &mp_opt);
 
 	/* The subflow can be in close state only if check_fully_established()
 	 * just sent a reset. If so, tell the caller to ignore the current packet.
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index cb1862ca5cb3..8f8b4173a566 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -944,7 +944,7 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
 				 const struct mptcp_options_received *mp_opt,
 				 struct sock *ssk,
 				 struct request_sock *req);
-void mptcp_get_options(const struct sk_buff *skb,
+void mptcp_get_options(const struct sock *sk, const struct sk_buff *skb,
 		       struct mptcp_options_received *mp_opt);
 
 void mptcp_finish_connect(struct sock *sk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index b1aa772a2519..d0ff0fb83b29 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -202,7 +202,7 @@ static int subflow_check_req(struct request_sock *req,
 	}
 #endif
 
-	mptcp_get_options(skb, &mp_opt);
+	mptcp_get_options(sk_listener, skb, &mp_opt);
 
 	opt_mp_capable = !!(mp_opt.suboptions & OPTION_MPTCP_MPC_SYN);
 	opt_mp_join = !!(mp_opt.suboptions & OPTION_MPTCP_MPJ_SYN);
@@ -308,7 +308,7 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req,
 	int err;
 
 	subflow_init_req(req, sk_listener);
-	mptcp_get_options(skb, &mp_opt);
+	mptcp_get_options(sk_listener, skb, &mp_opt);
 
 	opt_mp_capable = !!(mp_opt.suboptions & OPTION_MPTCP_MPC_ACK);
 	opt_mp_join = !!(mp_opt.suboptions & OPTION_MPTCP_MPJ_ACK);
@@ -579,7 +579,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 	subflow->ssn_offset = TCP_SKB_CB(skb)->seq;
 	pr_debug("subflow=%p synack seq=%x\n", subflow, subflow->ssn_offset);
 
-	mptcp_get_options(skb, &mp_opt);
+	mptcp_get_options(sk, skb, &mp_opt);
 	if (subflow->request_mptcp) {
 		if (!(mp_opt.suboptions & OPTION_MPTCP_MPC_SYNACK)) {
 			if (!mptcp_try_fallback(sk,
@@ -881,13 +881,13 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		 * reordered MPC will cause fallback, but we don't have other
 		 * options.
 		 */
-		mptcp_get_options(skb, &mp_opt);
+		mptcp_get_options(sk, skb, &mp_opt);
 		if (!(mp_opt.suboptions &
 		      (OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_ACK)))
 			fallback = true;
 
 	} else if (subflow_req->mp_join) {
-		mptcp_get_options(skb, &mp_opt);
+		mptcp_get_options(sk, skb, &mp_opt);
 		if (!(mp_opt.suboptions & OPTION_MPTCP_MPJ_ACK))
 			fallback = true;
 	}

-- 
2.55.0

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

[Matthieu Baerts (NGI0)]

When an invalid option is detected -- any unexpected suboptions
combinations or use of invalid sizes -- the current behaviour is to
ignore the currently parsed option.

Receiving such invalid options is suspicious: either this is coming from
a buggy host, or an attacker. In these cases, it seems safer to drop any
previously parsed MPTCP option from such packets. This will likely lead
to a fallback to TCP or a subflow reject, which seems to be a good
reaction in these situations.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
v2: break the while-loop, not the switch-case.
---
 net/mptcp/options.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 10a150e2ad69..b068f4f1861e 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 sock *sk, const struct sk_buff *skb,
+static bool mptcp_parse_option(const struct sock *sk, const struct sk_buff *skb,
 			       const unsigned char *ptr, int opsize,
 			       struct mptcp_options_received *mp_opt)
 {
@@ -214,14 +214,8 @@ static void mptcp_parse_option(const struct sock *sk, const struct sk_buff *skb,
 		 * RFC 8684 Section 3.3.0 checks later in subflow_data_ready
 		 */
 		if (opsize != expected_opsize &&
-		    opsize != expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM) {
-			mp_opt->dsn64 = 0;
-			mp_opt->use_map = 0;
-			mp_opt->ack64 = 0;
-			mp_opt->use_ack = 0;
-			mp_opt->data_fin = 0;
+		    opsize != expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM)
 			goto invalid;
-		}
 
 		mp_opt->suboptions |= OPTION_MPTCP_DSS;
 		if (mp_opt->use_ack) {
@@ -424,10 +418,11 @@ static void mptcp_parse_option(const struct sock *sk, const struct sk_buff *skb,
 		break;
 	}
 
-	return;
+	return true;
 
 invalid:
 	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OPTINVALID);
+	return false;
 }
 
 void mptcp_get_options(const struct sock *sk, const struct sk_buff *skb,
@@ -464,9 +459,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;
+				return;
+			}
 			ptr += opsize - 2;
 			length -= opsize;
 		}

-- 
2.55.0

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

[sashiko-bot]

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

Re: [PATCH mptcp-next v3 0/3] mptcp: options: MIB for invalid & drop suboptions

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/40958087376

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/7d5e686a6cfc
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1196631


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next v3 1/3] mptcp: options: use a dedicated bit for csum reqd

[Geliang Tang]

Hi Matt,

On Thu, 2026-10-29 at 22:05 +0100, Matthieu Baerts (NGI0) wrote:
> Instead of mixing that with suboptions: that's the only non-
> suboptions
> that is present in the 'suboptions' field.
> 
> This even cause a workaround with OPTIONS_MPTCP_DSS to exclude this
> non-suboptions when checking which other ones are set.

I initially thought about splitting this patch into two: one to remove
OPTION_MPTCP_CSUMREQD, and another to remove OPTIONS_MPTCP_DSS. But on
second thought, it's better to keep them together as is.

> 
> Move it to a dedicated free bit, and adapt the corresponding code.
> 
> This is clearer like that.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Looks good!

	Reviewed-by: Geliang Tang <geliang@kernel.org>

> ---
>  net/mptcp/options.c  | 25 ++++++++++++-------------
>  net/mptcp/protocol.c |  2 +-
>  net/mptcp/protocol.h |  6 ++----
>  net/mptcp/subflow.c  |  2 +-
>  4 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index adbf8e0773db..cbc105199073 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -93,9 +93,8 @@ static void mptcp_parse_option(const struct sk_buff
> *skb,
>  		 * In other words, the only way for checksums not to
> be used
>  		 * is if both hosts in their SYNs set A=0."
>  		 */
> -		if ((flags & MPTCP_CAP_CHECKSUM_REQD) &&
> -		    opsize < TCPOLEN_MPTCP_MPC_ACK_DATA)
> -			mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
> +		mp_opt->csum_reqd = (flags &
> MPTCP_CAP_CHECKSUM_REQD) &&
> +				    opsize <
> TCPOLEN_MPTCP_MPC_ACK_DATA;
>  
>  		mp_opt->deny_join_id0 = !!(flags &
> MPTCP_CAP_DENY_JOIN_ID0);
>  
> @@ -122,7 +121,7 @@ static void mptcp_parse_option(const struct
> sk_buff *skb,
>  		}
>  		if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM) {
>  			mp_opt->csum = get_unaligned((__force
> __sum16 *)ptr);
> -			mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
> +			mp_opt->csum_reqd = 1;
>  			ptr += 2;
>  		}
>  		pr_debug("MP_CAPABLE version=%x, flags=%x, optlen=%d
> sndr=%llu, rcvr=%llu len=%d csum=%u\n",
> @@ -251,22 +250,22 @@ static void mptcp_parse_option(const struct
> sk_buff *skb,
>  			ptr += 2;
>  
>  			if (opsize == expected_opsize +
> TCPOLEN_MPTCP_DSS_CHECKSUM) {
> -				mp_opt->suboptions |=
> OPTION_MPTCP_CSUMREQD;
>  				mp_opt->csum =
> get_unaligned((__force __sum16 *)ptr);
> +				mp_opt->csum_reqd = 1;
>  				ptr += 2;
>  			}
>  
>  			pr_debug("data_seq=%llu subflow_seq=%u
> data_len=%u csum=%d:%u\n",
>  				 mp_opt->data_seq, mp_opt-
> >subflow_seq,
> -				 mp_opt->data_len, !!(mp_opt-
> >suboptions & OPTION_MPTCP_CSUMREQD),
> -				 (mp_opt->suboptions &
> OPTION_MPTCP_CSUMREQD) ? mp_opt->csum : 0);
> +				 mp_opt->data_len, mp_opt-
> >csum_reqd,
> +				 mp_opt->csum_reqd ? mp_opt->csum :
> 0);
>  		}
>  
>  		break;
>  
>  	case MPTCPOPT_ADD_ADDR:
>  		/* Can be used with a restricted number of other
> options */
> -		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_DSS |
> +		if ((mp_opt->suboptions & ~(OPTION_MPTCP_DSS |
>  					    OPTION_MPTCP_RM_ADDR |
>  					    OPTION_MPTCP_PRIO)) !=
> 0)
>  			break;
> @@ -333,7 +332,7 @@ 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_MPC_ACK |
>  					    OPTIONS_MPTCP_MPJ |
> -					    OPTIONS_MPTCP_DSS |
> +					    OPTION_MPTCP_DSS |
>  					    OPTION_MPTCP_ADD_ADDR |
>  					    OPTION_MPTCP_PRIO)) !=
> 0)
>  			break;
> @@ -354,7 +353,7 @@ static void mptcp_parse_option(const struct
> sk_buff *skb,
>  	case MPTCPOPT_MP_PRIO:
>  		/* Can be used with a restricted number of other
> options */
>  		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_MPJ |
> -					    OPTIONS_MPTCP_DSS |
> +					    OPTION_MPTCP_DSS |
>  					    OPTION_MPTCP_ADD_ADDR |
>  					    OPTION_MPTCP_RM_ADDR))
> != 0)
>  			break;
> @@ -369,7 +368,7 @@ static void mptcp_parse_option(const struct
> sk_buff *skb,
>  
>  	case MPTCPOPT_MP_FASTCLOSE:
>  		/* Can be used with a restricted number of other
> options */
> -		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_DSS |
> +		if ((mp_opt->suboptions & ~(OPTION_MPTCP_DSS |
>  					    OPTION_MPTCP_RST)) != 0)
>  			break;
>  
> @@ -405,7 +404,7 @@ static void mptcp_parse_option(const struct
> sk_buff *skb,
>  
>  	case MPTCPOPT_MP_FAIL:
>  		/* Can be used with a restricted number of other
> options */
> -		if ((mp_opt->suboptions & ~(OPTIONS_MPTCP_DSS |
> +		if ((mp_opt->suboptions & ~(OPTION_MPTCP_DSS |
>  					    OPTION_MPTCP_RST)) != 0)
>  			break;
>  
> @@ -1352,7 +1351,7 @@ bool mptcp_incoming_options(struct sock *sk,
> struct sk_buff *skb)
>  		}
>  		mpext->data_len = mp_opt.data_len;
>  		mpext->use_map = 1;
> -		mpext->csum_reqd = !!(mp_opt.suboptions &
> OPTION_MPTCP_CSUMREQD);
> +		mpext->csum_reqd = mp_opt.csum_reqd;
>  
>  		if (mpext->csum_reqd)
>  			mpext->csum = mp_opt.csum;
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8..c85401952bf1 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3798,7 +3798,7 @@ struct sock *mptcp_sk_clone_init(const struct
> sock *sk,
>  	WRITE_ONCE(msk->token, subflow_req->token);
>  	msk->in_accept_queue = 1;
>  	WRITE_ONCE(msk->fully_established, false);
> -	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
> +	if (mp_opt->csum_reqd)
>  		WRITE_ONCE(msk->csum_enabled, true);
>  
>  	WRITE_ONCE(msk->write_seq, subflow_req->idsn + 1);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 3be60da12924..cb1862ca5cb3 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -31,13 +31,10 @@
>  #define OPTION_MPTCP_DSS	BIT(11)
>  #define OPTION_MPTCP_FAIL	BIT(12)
>  
> -#define OPTION_MPTCP_CSUMREQD	BIT(13)
> -
>  #define OPTIONS_MPTCP_MPC	(OPTION_MPTCP_MPC_SYN |
> OPTION_MPTCP_MPC_SYNACK | \
>  				 OPTION_MPTCP_MPC_ACK)
>  #define OPTIONS_MPTCP_MPJ	(OPTION_MPTCP_MPJ_SYN |
> OPTION_MPTCP_MPJ_SYNACK | \
>  				 OPTION_MPTCP_MPJ_ACK)
> -#define OPTIONS_MPTCP_DSS	(OPTION_MPTCP_DSS |
> OPTION_MPTCP_CSUMREQD)
>  
>  /* MPTCP option subtypes */
>  #define MPTCPOPT_MP_CAPABLE	0
> @@ -165,7 +162,8 @@ struct mptcp_options_received {
>  		    echo:1,
>  		    backup:1,
>  		    deny_join_id0:1,
> -		    __unused:2;
> +		    csum_reqd:1,
> +		    __unused:1;
>  	);
>  	struct mptcp_rm_list rm_list;
>  	/* Options below are mutually exclusive, see
> mptcp_parse_option() */
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b20258..b1aa772a2519 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -592,7 +592,7 @@ static void subflow_finish_connect(struct sock
> *sk, const struct sk_buff *skb)
>  			goto fallback;
>  		}
>  
> -		if (mp_opt.suboptions & OPTION_MPTCP_CSUMREQD)
> +		if (mp_opt.csum_reqd)
>  			WRITE_ONCE(msk->csum_enabled, true);
>  		if (mp_opt.deny_join_id0)
>  			WRITE_ONCE(msk->pm.remote_deny_join_id0,
> true);

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

[Geliang Tang]

On Thu, 2026-10-29 at 22:05 +0100, Matthieu Baerts (NGI0) wrote:
> When an invalid option is detected -- any unexpected suboptions
> combinations or use of invalid sizes -- the current behaviour is to
> ignore the currently parsed option.
> 
> Receiving such invalid options is suspicious: either this is coming
> from
> a buggy host, or an attacker. In these cases, it seems safer to drop
> any
> previously parsed MPTCP option from such packets. This will likely
> lead
> to a fallback to TCP or a subflow reject, which seems to be a good
> reaction in these situations.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> v2: break the while-loop, not the switch-case.
> ---
>  net/mptcp/options.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 10a150e2ad69..b068f4f1861e 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 sock *sk, const struct
> sk_buff *skb,
> +static bool mptcp_parse_option(const struct sock *sk, const struct
> sk_buff *skb,
>  			       const unsigned char *ptr, int opsize,
>  			       struct mptcp_options_received
> *mp_opt)
>  {
> @@ -214,14 +214,8 @@ static void mptcp_parse_option(const struct sock
> *sk, const struct sk_buff *skb,
>  		 * RFC 8684 Section 3.3.0 checks later in
> subflow_data_ready
>  		 */
>  		if (opsize != expected_opsize &&
> -		    opsize != expected_opsize +
> TCPOLEN_MPTCP_DSS_CHECKSUM) {
> -			mp_opt->dsn64 = 0;
> -			mp_opt->use_map = 0;
> -			mp_opt->ack64 = 0;
> -			mp_opt->use_ack = 0;
> -			mp_opt->data_fin = 0;
> +		    opsize != expected_opsize +
> TCPOLEN_MPTCP_DSS_CHECKSUM)
>  			goto invalid;
> -		}
>  
>  		mp_opt->suboptions |= OPTION_MPTCP_DSS;
>  		if (mp_opt->use_ack) {
> @@ -424,10 +418,11 @@ static void mptcp_parse_option(const struct
> sock *sk, const struct sk_buff *skb,
>  		break;
>  	}
>  
> -	return;
> +	return true;
>  
>  invalid:
>  	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OPTINVALID);
> +	return false;
>  }
>  
>  void mptcp_get_options(const struct sock *sk, const struct sk_buff
> *skb,
> @@ -464,9 +459,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;

I hesitated at first on why clearing status alone is sufficient to
replace clearing all the individual bit fields. After a closer look at
struct_group(status, ...), it covers suboptions and all the bit fields
(use_map, dsn64, data_fin, use_ack, ack64, etc.) in one shot - more
thorough than the old per-field clearing, and better for future
extensibility as new fields added to the group are automatically
covered.

So this patch looks good.

	Reviewed-by: Geliang Tang <geliang@kernel.org>

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

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

[Geliang Tang]

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?