[PATCH mptcp-net 0/3] mptcp: avoid uninitialised read with MPJ with SYN cookies

[PATCH mptcp-net 0/3] mptcp: avoid uninitialised read with MPJ with SYN cookies

[Matthieu Baerts (NGI0)]

This is a follow-up of [1], but addressing even less problematic
entries.

The first patch saves request_bkup, the second one avoids a possible
(harmless) KMSAN warning, and the 3rd one is a cleanup for next.

Link: https://lore.kernel.org/20260815115205.197151-1-harshitvaru666@gmail.com
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (3):
      mptcp: syncookies: remember the request backup flag
      mptcp: subflow: no need to copy thmac during ulp_clone
      [next] mptcp: remove thmac from subflow ctx

 net/mptcp/protocol.h   |  1 -
 net/mptcp/subflow.c    | 19 +++++++++----------
 net/mptcp/syncookies.c |  5 ++++-
 3 files changed, 13 insertions(+), 12 deletions(-)
---
base-commit: 61862bf50d532e8a538221643cd75d25a6f4f0f4
change-id: 20260816-mptcp-cook-reqb-6f35d027cd15

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

[PATCH mptcp-net 1/3] mptcp: syncookies: remember the request backup flag

[Matthieu Baerts (NGI0)]

Instead of using an uninitialised bit when copying the info in
subflow_ulp_clone().

To fix this, no need to extend the join_entry structure: backup is
coming from struct mptcp_subflow_request_sock, only one bit. Do the same
here by using one bit for both.

Fixes: 9466a1ccebbe ("mptcp: enable JOIN requests even if cookies are in use")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Sashiko: yes, thmac is also copied in ulp_clone and not init in
mptcp_token_join_cookie_init_state, see the next patch.
---
 net/mptcp/syncookies.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/syncookies.c b/net/mptcp/syncookies.c
index b5cac5701122..9474706641c1 100644
--- a/net/mptcp/syncookies.c
+++ b/net/mptcp/syncookies.c
@@ -26,7 +26,8 @@ struct join_entry {
 	u32 local_nonce;
 	u8 join_id;
 	u8 local_id;
-	u8 backup;
+	u8 backup:1,
+	   request_bkup:1;
 	u8 valid;
 };
 
@@ -63,6 +64,7 @@ static void mptcp_join_store_state(struct join_entry *entry,
 	entry->remote_nonce = subflow_req->remote_nonce;
 	entry->local_nonce = subflow_req->local_nonce;
 	entry->backup = subflow_req->backup;
+	entry->request_bkup = subflow_req->request_bkup;
 	entry->join_id = subflow_req->remote_id;
 	entry->local_id = subflow_req->local_id;
 	entry->valid = 1;
@@ -117,6 +119,7 @@ bool mptcp_token_join_cookie_init_state(struct mptcp_subflow_request_sock *subfl
 	subflow_req->remote_nonce = e->remote_nonce;
 	subflow_req->local_nonce = e->local_nonce;
 	subflow_req->backup = e->backup;
+	subflow_req->request_bkup = e->request_bkup;
 	subflow_req->remote_id = e->join_id;
 	subflow_req->local_id = e->local_id;
 	subflow_req->token = e->token;

-- 
2.53.0

[PATCH mptcp-net 2/3] mptcp: subflow: no need to copy thmac during ulp_clone

[Matthieu Baerts (NGI0)]

'thmac' is not used after that point.

Indeed, subflow_ulp_clone() is called when the request on the passive
side is over, so when the truncated HMAC is no longer needed.

Note that in case of SYN cookies, thmac will not be initialised. So
better to remove it to avoid a warning for reading uninitialised data.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/subflow.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index af81ad5e699d..01db7edce18a 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -2084,7 +2084,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
 		new_ctx->request_bkup = subflow_req->request_bkup;
 		WRITE_ONCE(new_ctx->remote_id, subflow_req->remote_id);
 		new_ctx->token = subflow_req->token;
-		new_ctx->thmac = subflow_req->thmac;
 
 		/* the subflow req id is valid, fetched via subflow_check_req()
 		 * and subflow_token_join_request()

-- 
2.53.0

[PATCH next mptcp-net 3/3] mptcp: remove thmac from subflow ctx

[Matthieu Baerts (NGI0)]

This entry is only used in subflow_finish_connect().

Instead, use the original value from mp_opt, and pass it to
subflow_thmac_valid() to do the validation with the given truncated
hmac.

While at it, rename the variables in subflow_thmac_valid() to avoid
confusions about the received one vs the expected one.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/protocol.h |  1 -
 net/mptcp/subflow.c  | 18 +++++++++---------
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 7e168e450fb0..d414065d1966 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -592,7 +592,6 @@ struct mptcp_subflow_context {
 	bool	fully_established;  /* path validated */
 	u32	lent_mem_frag;
 	u32	remote_nonce;
-	u64	thmac;
 	u32	local_nonce;
 	u32	remote_token;
 	union {
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 01db7edce18a..2d7ccb01d234 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -408,20 +408,21 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
 #endif
 
 /* validate received truncated hmac and create hmac for third ACK */
-static bool subflow_thmac_valid(struct mptcp_subflow_context *subflow)
+static bool subflow_thmac_valid(struct mptcp_subflow_context *subflow,
+				u64 thmac)
 {
 	u8 hmac[SHA256_DIGEST_SIZE];
-	u64 thmac;
+	u64 expected_thmac;
 
 	subflow_generate_hmac(subflow->remote_key, subflow->local_key,
 			      subflow->remote_nonce, subflow->local_nonce,
 			      hmac);
 
-	thmac = get_unaligned_be64(hmac);
-	pr_debug("subflow=%p, token=%u, thmac=%llu, subflow->thmac=%llu\n",
-		 subflow, subflow->token, thmac, subflow->thmac);
+	expected_thmac = get_unaligned_be64(hmac);
+	pr_debug("subflow=%p, token=%u, expected_thmac=%llu, thmac=%llu\n",
+		 subflow, subflow->token, expected_thmac, thmac);
 
-	return thmac == subflow->thmac;
+	return expected_thmac == thmac;
 }
 
 void mptcp_subflow_reset(struct sock *ssk)
@@ -571,14 +572,13 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 		}
 
 		subflow->backup = mp_opt.backup;
-		subflow->thmac = mp_opt.thmac;
 		subflow->remote_nonce = mp_opt.nonce;
 		WRITE_ONCE(subflow->remote_id, mp_opt.join_id);
 		pr_debug("subflow=%p, thmac=%llu, remote_nonce=%u backup=%d\n",
-			 subflow, subflow->thmac, subflow->remote_nonce,
+			 subflow, mp_opt.thmac, subflow->remote_nonce,
 			 subflow->backup);
 
-		if (!subflow_thmac_valid(subflow)) {
+		if (!subflow_thmac_valid(subflow, mp_opt.thmac)) {
 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_JOINSYNACKMAC);
 			subflow->reset_reason = MPTCP_RST_EMPTCP;
 			goto do_reset;

-- 
2.53.0

Re: [PATCH mptcp-net 0/3] mptcp: avoid uninitialised read with MPJ with SYN cookies

[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): Unstable: 1 failed test(s): selftest_mptcp_join ⚠️ 
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/32171215149

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


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 next mptcp-net 3/3] mptcp: remove thmac from subflow ctx

[Geliang Tang]

Hi Matt,

On Tue, 2026-08-18 at 19:55 +0200, Matthieu Baerts (NGI0) wrote:
> This entry is only used in subflow_finish_connect().
> 
> Instead, use the original value from mp_opt, and pass it to
> subflow_thmac_valid() to do the validation with the given truncated
> hmac.
> 
> While at it, rename the variables in subflow_thmac_valid() to avoid
> confusions about the received one vs the expected one.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/protocol.h |  1 -
>  net/mptcp/subflow.c  | 18 +++++++++---------
>  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 7e168e450fb0..d414065d1966 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -592,7 +592,6 @@ struct mptcp_subflow_context {
>  	bool	fully_established;  /* path validated */
>  	u32	lent_mem_frag;
>  	u32	remote_nonce;
> -	u64	thmac;
>  	u32	local_nonce;
>  	u32	remote_token;
>  	union {
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 01db7edce18a..2d7ccb01d234 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -408,20 +408,21 @@ static struct dst_entry
> *subflow_v6_route_req(const struct sock *sk,
>  #endif
>  
>  /* validate received truncated hmac and create hmac for third ACK */
> -static bool subflow_thmac_valid(struct mptcp_subflow_context
> *subflow)
> +static bool subflow_thmac_valid(struct mptcp_subflow_context
> *subflow,
> +				u64 thmac)

Perhaps we could rename this parameter, say to "subflow_thmac", so that
we don't need to rename the local variable "thmac" to "expected_thmac".
What do you think?

Thanks,
-Geliang

>  {
>  	u8 hmac[SHA256_DIGEST_SIZE];
> -	u64 thmac;
> +	u64 expected_thmac;
>  
>  	subflow_generate_hmac(subflow->remote_key, subflow-
> >local_key,
>  			      subflow->remote_nonce, subflow-
> >local_nonce,
>  			      hmac);
>  
> -	thmac = get_unaligned_be64(hmac);
> -	pr_debug("subflow=%p, token=%u, thmac=%llu, subflow-
> >thmac=%llu\n",
> -		 subflow, subflow->token, thmac, subflow->thmac);
> +	expected_thmac = get_unaligned_be64(hmac);
> +	pr_debug("subflow=%p, token=%u, expected_thmac=%llu,
> thmac=%llu\n",
> +		 subflow, subflow->token, expected_thmac, thmac);
>  
> -	return thmac == subflow->thmac;
> +	return expected_thmac == thmac;
>  }
>  
>  void mptcp_subflow_reset(struct sock *ssk)
> @@ -571,14 +572,13 @@ static void subflow_finish_connect(struct sock
> *sk, const struct sk_buff *skb)
>  		}
>  
>  		subflow->backup = mp_opt.backup;
> -		subflow->thmac = mp_opt.thmac;
>  		subflow->remote_nonce = mp_opt.nonce;
>  		WRITE_ONCE(subflow->remote_id, mp_opt.join_id);
>  		pr_debug("subflow=%p, thmac=%llu, remote_nonce=%u
> backup=%d\n",
> -			 subflow, subflow->thmac, subflow-
> >remote_nonce,
> +			 subflow, mp_opt.thmac, subflow-
> >remote_nonce,
>  			 subflow->backup);
>  
> -		if (!subflow_thmac_valid(subflow)) {
> +		if (!subflow_thmac_valid(subflow, mp_opt.thmac)) {
>  			MPTCP_INC_STATS(sock_net(sk),
> MPTCP_MIB_JOINSYNACKMAC);
>  			subflow->reset_reason = MPTCP_RST_EMPTCP;
>  			goto do_reset;

Re: [PATCH next mptcp-net 3/3] mptcp: remove thmac from subflow ctx

[Matthieu Baerts]

Hi Geliang,

On 19/08/2026 11:56, Geliang Tang wrote:
> Hi Matt,
> 
> On Tue, 2026-08-18 at 19:55 +0200, Matthieu Baerts (NGI0) wrote:
>> This entry is only used in subflow_finish_connect().
>>
>> Instead, use the original value from mp_opt, and pass it to
>> subflow_thmac_valid() to do the validation with the given truncated
>> hmac.
>>
>> While at it, rename the variables in subflow_thmac_valid() to avoid
>> confusions about the received one vs the expected one.
>>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>> ---
>>  net/mptcp/protocol.h |  1 -
>>  net/mptcp/subflow.c  | 18 +++++++++---------
>>  2 files changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index 7e168e450fb0..d414065d1966 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -592,7 +592,6 @@ struct mptcp_subflow_context {
>>  	bool	fully_established;  /* path validated */
>>  	u32	lent_mem_frag;
>>  	u32	remote_nonce;
>> -	u64	thmac;
>>  	u32	local_nonce;
>>  	u32	remote_token;
>>  	union {
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 01db7edce18a..2d7ccb01d234 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -408,20 +408,21 @@ static struct dst_entry
>> *subflow_v6_route_req(const struct sock *sk,
>>  #endif
>>  
>>  /* validate received truncated hmac and create hmac for third ACK */
>> -static bool subflow_thmac_valid(struct mptcp_subflow_context
>> *subflow)
>> +static bool subflow_thmac_valid(struct mptcp_subflow_context
>> *subflow,
>> +				u64 thmac)
> 
> Perhaps we could rename this parameter, say to "subflow_thmac", so that
> we don't need to rename the local variable "thmac" to "expected_thmac".
> What do you think?
I initially did that, but it fell more natural to use the "expected"
keyword, similar to other places, and in the selftests, than the
opposite. Also, we pass "thmac" to subflow_thmac_valid(), but seeing the
argument renamed didn't feel right.

If it was for -net, I would have minimised the diff, but here, let's get
thing rights I think, no?

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