[PATCH net 0/5] mptcp: better validation of MPTCPOPT_MP_JOIN option

[PATCH net 0/5] mptcp: better validation of MPTCPOPT_MP_JOIN option

[Eric Dumazet]

Based on a syzbot report (see 4th patch in the series).

We need to be more explicit about which one of the
following flag is set by mptcp_parse_option():

- OPTION_MPTCP_MPJ_SYN
- OPTION_MPTCP_MPJ_SYNACK
- OPTION_MPTCP_MPJ_ACK

Then select the appropriate values instead of OPTIONS_MPTCP_MPJ

Paolo suggested to do the same for OPTIONS_MPTCP_MPC (5th patch)

Eric Dumazet (5):
  mptcp: mptcp_parse_option() fix for MPTCPOPT_MP_JOIN
  mptcp: strict validation before using mp_opt->hmac
  mptcp: use OPTION_MPTCP_MPJ_SYNACK in subflow_finish_connect()
  mptcp: use OPTION_MPTCP_MPJ_SYN in subflow_check_req()
  mptcp: refine opt_mp_capable determination

 net/mptcp/options.c |  6 +++---
 net/mptcp/subflow.c | 16 ++++++++--------
 2 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.43.0.275.g3460e3d667-goog

[PATCH net 1/5] mptcp: mptcp_parse_option() fix for MPTCPOPT_MP_JOIN

[Eric Dumazet]

mptcp_parse_option() currently sets OPTIONS_MPTCP_MPJ, for the three
possible cases handled for MPTCPOPT_MP_JOIN option.

OPTIONS_MPTCP_MPJ is the combination of three flags:
- OPTION_MPTCP_MPJ_SYN
- OPTION_MPTCP_MPJ_SYNACK
- OPTION_MPTCP_MPJ_ACK

This is a problem, because backup, join_id, token, nonce and/or hmac fields
could be left uninitialized in some cases.

Distinguish the three cases, as following patches will need this step.

Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Florian Westphal <fw@strlen.de>
Cc: Peter Krystad <peter.krystad@linux.intel.com>
Cc: Matthieu Baerts <matttbe@kernel.org>
Cc: Mat Martineau <martineau@kernel.org>
Cc: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/options.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c53914012d01d38c2dc0a3578bf3651595956e72..d2527d189a799319c068a5b76a5816cc7a905861 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -123,8 +123,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		break;
 
 	case MPTCPOPT_MP_JOIN:
-		mp_opt->suboptions |= OPTIONS_MPTCP_MPJ;
 		if (opsize == TCPOLEN_MPTCP_MPJ_SYN) {
+			mp_opt->suboptions |= OPTION_MPTCP_MPJ_SYN;
 			mp_opt->backup = *ptr++ & MPTCPOPT_BACKUP;
 			mp_opt->join_id = *ptr++;
 			mp_opt->token = get_unaligned_be32(ptr);
@@ -135,6 +135,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 				 mp_opt->backup, mp_opt->join_id,
 				 mp_opt->token, mp_opt->nonce);
 		} else if (opsize == TCPOLEN_MPTCP_MPJ_SYNACK) {
+			mp_opt->suboptions |= OPTION_MPTCP_MPJ_SYNACK;
 			mp_opt->backup = *ptr++ & MPTCPOPT_BACKUP;
 			mp_opt->join_id = *ptr++;
 			mp_opt->thmac = get_unaligned_be64(ptr);
@@ -145,11 +146,10 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 				 mp_opt->backup, mp_opt->join_id,
 				 mp_opt->thmac, mp_opt->nonce);
 		} else if (opsize == TCPOLEN_MPTCP_MPJ_ACK) {
+			mp_opt->suboptions |= OPTION_MPTCP_MPJ_ACK;
 			ptr += 2;
 			memcpy(mp_opt->hmac, ptr, MPTCPOPT_HMAC_LEN);
 			pr_debug("MP_JOIN hmac");
-		} else {
-			mp_opt->suboptions &= ~OPTIONS_MPTCP_MPJ;
 		}
 		break;
 
-- 
2.43.0.275.g3460e3d667-goog

[PATCH net 2/5] mptcp: strict validation before using mp_opt->hmac

[Eric Dumazet]

mp_opt->hmac contains uninitialized data unless OPTION_MPTCP_MPJ_ACK
was set in mptcp_parse_option().

We must refine the condition before we call subflow_hmac_valid().

Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Florian Westphal <fw@strlen.de>
Cc: Peter Krystad <peter.krystad@linux.intel.com>
Cc: Matthieu Baerts <matttbe@kernel.org>
Cc: Mat Martineau <martineau@kernel.org>
Cc: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/subflow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 3eacd04e7099e6de1a161c176a74959722445286..bb05477006a6ea111b7fc79645099dfa924e4135 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -788,7 +788,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 
 	} else if (subflow_req->mp_join) {
 		mptcp_get_options(skb, &mp_opt);
-		if (!(mp_opt.suboptions & OPTIONS_MPTCP_MPJ) ||
+		if (!(mp_opt.suboptions & OPTION_MPTCP_MPJ_ACK) ||
 		    !subflow_hmac_valid(req, &mp_opt) ||
 		    !mptcp_can_accept_new_subflow(subflow_req->msk)) {
 			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKMAC);
-- 
2.43.0.275.g3460e3d667-goog

[PATCH net 3/5] mptcp: use OPTION_MPTCP_MPJ_SYNACK in subflow_finish_connect()

[Eric Dumazet]

subflow_finish_connect() uses four fields (backup, join_id, thmac, none)
that may contain garbage unless OPTION_MPTCP_MPJ_SYNACK has been set
in mptcp_parse_option()

Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Florian Westphal <fw@strlen.de>
Cc: Peter Krystad <peter.krystad@linux.intel.com>
Cc: Matthieu Baerts <matttbe@kernel.org>
Cc: Mat Martineau <martineau@kernel.org>
Cc: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/subflow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index bb05477006a6ea111b7fc79645099dfa924e4135..103ff5471993a8490c4e1e77f404ec7df4e2c6e3 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -506,7 +506,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 	} else if (subflow->request_join) {
 		u8 hmac[SHA256_DIGEST_SIZE];
 
-		if (!(mp_opt.suboptions & OPTIONS_MPTCP_MPJ)) {
+		if (!(mp_opt.suboptions & OPTION_MPTCP_MPJ_SYNACK)) {
 			subflow->reset_reason = MPTCP_RST_EMPTCP;
 			goto do_reset;
 		}
-- 
2.43.0.275.g3460e3d667-goog

[PATCH net 4/5] mptcp: use OPTION_MPTCP_MPJ_SYN in subflow_check_req()

[Eric Dumazet]

syzbot reported that subflow_check_req() was using uninitialized data in
subflow_check_req() [1]

This is because mp_opt.token is only set when OPTION_MPTCP_MPJ_SYN is also set.

While we are are it, fix mptcp_subflow_init_cookie_req()
to test for OPTION_MPTCP_MPJ_ACK.

[1]

BUG: KMSAN: uninit-value in subflow_token_join_request net/mptcp/subflow.c:91 [inline]
 BUG: KMSAN: uninit-value in subflow_check_req+0x1028/0x15d0 net/mptcp/subflow.c:209
  subflow_token_join_request net/mptcp/subflow.c:91 [inline]
  subflow_check_req+0x1028/0x15d0 net/mptcp/subflow.c:209
  subflow_v6_route_req+0x269/0x410 net/mptcp/subflow.c:367
  tcp_conn_request+0x153a/0x4240 net/ipv4/tcp_input.c:7164
 subflow_v6_conn_request+0x3ee/0x510
  tcp_rcv_state_process+0x2e1/0x4ac0 net/ipv4/tcp_input.c:6659
  tcp_v6_do_rcv+0x11bf/0x1fe0 net/ipv6/tcp_ipv6.c:1669
  tcp_v6_rcv+0x480b/0x4fb0 net/ipv6/tcp_ipv6.c:1900
  ip6_protocol_deliver_rcu+0xda6/0x2a60 net/ipv6/ip6_input.c:438
  ip6_input_finish net/ipv6/ip6_input.c:483 [inline]
  NF_HOOK include/linux/netfilter.h:314 [inline]
  ip6_input+0x15d/0x430 net/ipv6/ip6_input.c:492
  dst_input include/net/dst.h:461 [inline]
  ip6_rcv_finish+0x5db/0x870 net/ipv6/ip6_input.c:79
  NF_HOOK include/linux/netfilter.h:314 [inline]
  ipv6_rcv+0xda/0x390 net/ipv6/ip6_input.c:310
  __netif_receive_skb_one_core net/core/dev.c:5532 [inline]
  __netif_receive_skb+0x1a6/0x5a0 net/core/dev.c:5646
  netif_receive_skb_internal net/core/dev.c:5732 [inline]
  netif_receive_skb+0x58/0x660 net/core/dev.c:5791
  tun_rx_batched+0x3ee/0x980 drivers/net/tun.c:1555
  tun_get_user+0x53af/0x66d0 drivers/net/tun.c:2002
  tun_chr_write_iter+0x3af/0x5d0 drivers/net/tun.c:2048
  call_write_iter include/linux/fs.h:2020 [inline]
  new_sync_write fs/read_write.c:491 [inline]
  vfs_write+0x8ef/0x1490 fs/read_write.c:584
  ksys_write+0x20f/0x4c0 fs/read_write.c:637
  __do_sys_write fs/read_write.c:649 [inline]
  __se_sys_write fs/read_write.c:646 [inline]
  __x64_sys_write+0x93/0xd0 fs/read_write.c:646
  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
  do_syscall_64+0x44/0x110 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x63/0x6b

Local variable mp_opt created at:
  subflow_check_req+0x6d/0x15d0 net/mptcp/subflow.c:145
  subflow_v6_route_req+0x269/0x410 net/mptcp/subflow.c:367

CPU: 1 PID: 5924 Comm: syz-executor.3 Not tainted 6.7.0-rc8-syzkaller-00055-g5eff55d725a4 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023

Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Florian Westphal <fw@strlen.de>
Cc: Peter Krystad <peter.krystad@linux.intel.com>
Cc: Matthieu Baerts <matttbe@kernel.org>
Cc: Mat Martineau <martineau@kernel.org>
Cc: Geliang Tang <geliang.tang@linux.dev>
---
 net/mptcp/subflow.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 103ff5471993a8490c4e1e77f404ec7df4e2c6e3..13039ac8d1ab641fb9b22b621ac011e6a7bc9e37 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -158,7 +158,7 @@ static int subflow_check_req(struct request_sock *req,
 	mptcp_get_options(skb, &mp_opt);
 
 	opt_mp_capable = !!(mp_opt.suboptions & OPTIONS_MPTCP_MPC);
-	opt_mp_join = !!(mp_opt.suboptions & OPTIONS_MPTCP_MPJ);
+	opt_mp_join = !!(mp_opt.suboptions & OPTION_MPTCP_MPJ_SYN);
 	if (opt_mp_capable) {
 		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVE);
 
@@ -255,7 +255,7 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req,
 	mptcp_get_options(skb, &mp_opt);
 
 	opt_mp_capable = !!(mp_opt.suboptions & OPTIONS_MPTCP_MPC);
-	opt_mp_join = !!(mp_opt.suboptions & OPTIONS_MPTCP_MPJ);
+	opt_mp_join = !!(mp_opt.suboptions & OPTION_MPTCP_MPJ_ACK);
 	if (opt_mp_capable && opt_mp_join)
 		return -EINVAL;
 
-- 
2.43.0.275.g3460e3d667-goog

[PATCH net 5/5] mptcp: refine opt_mp_capable determination

[Eric Dumazet]

OPTIONS_MPTCP_MPC is a combination of three flags.

It would be better to be strict about testing what
flag is expected, at least for code readability.

mptcp_parse_option() already makes the distinction.

- subflow_check_req() should use OPTION_MPTCP_MPC_SYN.

- mptcp_subflow_init_cookie_req() should use OPTION_MPTCP_MPC_ACK.

- subflow_finish_connect() should use OPTION_MPTCP_MPC_SYNACK

- subflow_syn_recv_sock should use OPTION_MPTCP_MPC_ACK

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/mptcp/subflow.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 13039ac8d1ab641fb9b22b621ac011e6a7bc9e37..1117d1e84274a5ea1ede990566f67c0073fd86a0 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -157,7 +157,7 @@ static int subflow_check_req(struct request_sock *req,
 
 	mptcp_get_options(skb, &mp_opt);
 
-	opt_mp_capable = !!(mp_opt.suboptions & OPTIONS_MPTCP_MPC);
+	opt_mp_capable = !!(mp_opt.suboptions & OPTION_MPTCP_MPC_SYN);
 	opt_mp_join = !!(mp_opt.suboptions & OPTION_MPTCP_MPJ_SYN);
 	if (opt_mp_capable) {
 		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVE);
@@ -254,7 +254,7 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req,
 	subflow_init_req(req, sk_listener);
 	mptcp_get_options(skb, &mp_opt);
 
-	opt_mp_capable = !!(mp_opt.suboptions & OPTIONS_MPTCP_MPC);
+	opt_mp_capable = !!(mp_opt.suboptions & OPTION_MPTCP_MPC_ACK);
 	opt_mp_join = !!(mp_opt.suboptions & OPTION_MPTCP_MPJ_ACK);
 	if (opt_mp_capable && opt_mp_join)
 		return -EINVAL;
@@ -486,7 +486,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
 
 	mptcp_get_options(skb, &mp_opt);
 	if (subflow->request_mptcp) {
-		if (!(mp_opt.suboptions & OPTIONS_MPTCP_MPC)) {
+		if (!(mp_opt.suboptions & OPTION_MPTCP_MPC_SYNACK)) {
 			MPTCP_INC_STATS(sock_net(sk),
 					MPTCP_MIB_MPCAPABLEACTIVEFALLBACK);
 			mptcp_do_fallback(sk);
@@ -783,7 +783,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		 * options.
 		 */
 		mptcp_get_options(skb, &mp_opt);
-		if (!(mp_opt.suboptions & OPTIONS_MPTCP_MPC))
+		if (!(mp_opt.suboptions & OPTION_MPTCP_MPC_ACK))
 			fallback = true;
 
 	} else if (subflow_req->mp_join) {
-- 
2.43.0.275.g3460e3d667-goog

Re: [PATCH net 0/5] mptcp: better validation of MPTCPOPT_MP_JOIN option

[Paolo Abeni]

On Thu, 2024-01-11 at 19:49 +0000, Eric Dumazet wrote:
> Based on a syzbot report (see 4th patch in the series).
> 
> We need to be more explicit about which one of the
> following flag is set by mptcp_parse_option():
> 
> - OPTION_MPTCP_MPJ_SYN
> - OPTION_MPTCP_MPJ_SYNACK
> - OPTION_MPTCP_MPJ_ACK
> 
> Then select the appropriate values instead of OPTIONS_MPTCP_MPJ
> 
> Paolo suggested to do the same for OPTIONS_MPTCP_MPC (5th patch)
> 
> Eric Dumazet (5):
>   mptcp: mptcp_parse_option() fix for MPTCPOPT_MP_JOIN
>   mptcp: strict validation before using mp_opt->hmac
>   mptcp: use OPTION_MPTCP_MPJ_SYNACK in subflow_finish_connect()
>   mptcp: use OPTION_MPTCP_MPJ_SYN in subflow_check_req()
>   mptcp: refine opt_mp_capable determination
> 
>  net/mptcp/options.c |  6 +++---
>  net/mptcp/subflow.c | 16 ++++++++--------
>  2 files changed, 11 insertions(+), 11 deletions(-)

The patches LGTM, thanks Eric!

I gave a spin into the mptcp self-tests and pktdrill and both suite are
happy.

Acked-by: Paolo Abeni <pabeni@redhat.com>

Re: [PATCH net 0/5] mptcp: better validation of MPTCPOPT_MP_JOIN option

[Mat Martineau]

On Thu, 11 Jan 2024, Eric Dumazet wrote:

> Based on a syzbot report (see 4th patch in the series).
>
> We need to be more explicit about which one of the
> following flag is set by mptcp_parse_option():
>
> - OPTION_MPTCP_MPJ_SYN
> - OPTION_MPTCP_MPJ_SYNACK
> - OPTION_MPTCP_MPJ_ACK
>
> Then select the appropriate values instead of OPTIONS_MPTCP_MPJ
>
> Paolo suggested to do the same for OPTIONS_MPTCP_MPC (5th patch)
>
> Eric Dumazet (5):
>  mptcp: mptcp_parse_option() fix for MPTCPOPT_MP_JOIN
>  mptcp: strict validation before using mp_opt->hmac
>  mptcp: use OPTION_MPTCP_MPJ_SYNACK in subflow_finish_connect()
>  mptcp: use OPTION_MPTCP_MPJ_SYN in subflow_check_req()
>  mptcp: refine opt_mp_capable determination
>
> net/mptcp/options.c |  6 +++---
> net/mptcp/subflow.c | 16 ++++++++--------
> 2 files changed, 11 insertions(+), 11 deletions(-)

Hi Eric -

Thanks for the fixes, code looks good (one separate commit message 
comment on patch 5):

Reviewed-by: Mat Martineau <martineau@kernel.org>

Re: [PATCH net 5/5] mptcp: refine opt_mp_capable determination

[Mat Martineau]

On Thu, 11 Jan 2024, Eric Dumazet wrote:

> OPTIONS_MPTCP_MPC is a combination of three flags.
>
> It would be better to be strict about testing what
> flag is expected, at least for code readability.
>
> mptcp_parse_option() already makes the distinction.
>
> - subflow_check_req() should use OPTION_MPTCP_MPC_SYN.
>
> - mptcp_subflow_init_cookie_req() should use OPTION_MPTCP_MPC_ACK.
>
> - subflow_finish_connect() should use OPTION_MPTCP_MPC_SYNACK
>
> - subflow_syn_recv_sock should use OPTION_MPTCP_MPC_ACK
>

Should this be applied to net-next?

If intended for -net, I think

Fixes: 74c7dfbee3e1 ("mptcp: consolidate in_opt sub-options fields in a bitmask")

is the appropriate commit to reference.


- Mat


> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> net/mptcp/subflow.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 13039ac8d1ab641fb9b22b621ac011e6a7bc9e37..1117d1e84274a5ea1ede990566f67c0073fd86a0 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -157,7 +157,7 @@ static int subflow_check_req(struct request_sock *req,
>
> 	mptcp_get_options(skb, &mp_opt);
>
> -	opt_mp_capable = !!(mp_opt.suboptions & OPTIONS_MPTCP_MPC);
> +	opt_mp_capable = !!(mp_opt.suboptions & OPTION_MPTCP_MPC_SYN);
> 	opt_mp_join = !!(mp_opt.suboptions & OPTION_MPTCP_MPJ_SYN);
> 	if (opt_mp_capable) {
> 		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVE);
> @@ -254,7 +254,7 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req,
> 	subflow_init_req(req, sk_listener);
> 	mptcp_get_options(skb, &mp_opt);
>
> -	opt_mp_capable = !!(mp_opt.suboptions & OPTIONS_MPTCP_MPC);
> +	opt_mp_capable = !!(mp_opt.suboptions & OPTION_MPTCP_MPC_ACK);
> 	opt_mp_join = !!(mp_opt.suboptions & OPTION_MPTCP_MPJ_ACK);
> 	if (opt_mp_capable && opt_mp_join)
> 		return -EINVAL;
> @@ -486,7 +486,7 @@ static void subflow_finish_connect(struct sock *sk, const struct sk_buff *skb)
>
> 	mptcp_get_options(skb, &mp_opt);
> 	if (subflow->request_mptcp) {
> -		if (!(mp_opt.suboptions & OPTIONS_MPTCP_MPC)) {
> +		if (!(mp_opt.suboptions & OPTION_MPTCP_MPC_SYNACK)) {
> 			MPTCP_INC_STATS(sock_net(sk),
> 					MPTCP_MIB_MPCAPABLEACTIVEFALLBACK);
> 			mptcp_do_fallback(sk);
> @@ -783,7 +783,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> 		 * options.
> 		 */
> 		mptcp_get_options(skb, &mp_opt);
> -		if (!(mp_opt.suboptions & OPTIONS_MPTCP_MPC))
> +		if (!(mp_opt.suboptions & OPTION_MPTCP_MPC_ACK))
> 			fallback = true;
>
> 	} else if (subflow_req->mp_join) {
> -- 
> 2.43.0.275.g3460e3d667-goog
>
>

Re: [PATCH net 1/5] mptcp: mptcp_parse_option() fix for MPTCPOPT_MP_JOIN

[Simon Horman]

On Thu, Jan 11, 2024 at 07:49:13PM +0000, Eric Dumazet wrote:
> mptcp_parse_option() currently sets OPTIONS_MPTCP_MPJ, for the three
> possible cases handled for MPTCPOPT_MP_JOIN option.
> 
> OPTIONS_MPTCP_MPJ is the combination of three flags:
> - OPTION_MPTCP_MPJ_SYN
> - OPTION_MPTCP_MPJ_SYNACK
> - OPTION_MPTCP_MPJ_ACK
> 
> This is a problem, because backup, join_id, token, nonce and/or hmac fields
> could be left uninitialized in some cases.
> 
> Distinguish the three cases, as following patches will need this step.
> 
> Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 2/5] mptcp: strict validation before using mp_opt->hmac

[Simon Horman]

On Thu, Jan 11, 2024 at 07:49:14PM +0000, Eric Dumazet wrote:
> mp_opt->hmac contains uninitialized data unless OPTION_MPTCP_MPJ_ACK
> was set in mptcp_parse_option().
> 
> We must refine the condition before we call subflow_hmac_valid().
> 
> Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 3/5] mptcp: use OPTION_MPTCP_MPJ_SYNACK in subflow_finish_connect()

[Simon Horman]

On Thu, Jan 11, 2024 at 07:49:15PM +0000, Eric Dumazet wrote:
> subflow_finish_connect() uses four fields (backup, join_id, thmac, none)
> that may contain garbage unless OPTION_MPTCP_MPJ_SYNACK has been set
> in mptcp_parse_option()
> 
> Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 4/5] mptcp: use OPTION_MPTCP_MPJ_SYN in subflow_check_req()

[Simon Horman]

On Thu, Jan 11, 2024 at 07:49:16PM +0000, Eric Dumazet wrote:
> syzbot reported that subflow_check_req() was using uninitialized data in
> subflow_check_req() [1]
> 
> This is because mp_opt.token is only set when OPTION_MPTCP_MPJ_SYN is also set.
> 
> While we are are it, fix mptcp_subflow_init_cookie_req()
> to test for OPTION_MPTCP_MPJ_ACK.
> 
> [1]
> 
> BUG: KMSAN: uninit-value in subflow_token_join_request net/mptcp/subflow.c:91 [inline]
>  BUG: KMSAN: uninit-value in subflow_check_req+0x1028/0x15d0 net/mptcp/subflow.c:209
>   subflow_token_join_request net/mptcp/subflow.c:91 [inline]
>   subflow_check_req+0x1028/0x15d0 net/mptcp/subflow.c:209
>   subflow_v6_route_req+0x269/0x410 net/mptcp/subflow.c:367
>   tcp_conn_request+0x153a/0x4240 net/ipv4/tcp_input.c:7164
>  subflow_v6_conn_request+0x3ee/0x510
>   tcp_rcv_state_process+0x2e1/0x4ac0 net/ipv4/tcp_input.c:6659
>   tcp_v6_do_rcv+0x11bf/0x1fe0 net/ipv6/tcp_ipv6.c:1669
>   tcp_v6_rcv+0x480b/0x4fb0 net/ipv6/tcp_ipv6.c:1900
>   ip6_protocol_deliver_rcu+0xda6/0x2a60 net/ipv6/ip6_input.c:438
>   ip6_input_finish net/ipv6/ip6_input.c:483 [inline]
>   NF_HOOK include/linux/netfilter.h:314 [inline]
>   ip6_input+0x15d/0x430 net/ipv6/ip6_input.c:492
>   dst_input include/net/dst.h:461 [inline]
>   ip6_rcv_finish+0x5db/0x870 net/ipv6/ip6_input.c:79
>   NF_HOOK include/linux/netfilter.h:314 [inline]
>   ipv6_rcv+0xda/0x390 net/ipv6/ip6_input.c:310
>   __netif_receive_skb_one_core net/core/dev.c:5532 [inline]
>   __netif_receive_skb+0x1a6/0x5a0 net/core/dev.c:5646
>   netif_receive_skb_internal net/core/dev.c:5732 [inline]
>   netif_receive_skb+0x58/0x660 net/core/dev.c:5791
>   tun_rx_batched+0x3ee/0x980 drivers/net/tun.c:1555
>   tun_get_user+0x53af/0x66d0 drivers/net/tun.c:2002
>   tun_chr_write_iter+0x3af/0x5d0 drivers/net/tun.c:2048
>   call_write_iter include/linux/fs.h:2020 [inline]
>   new_sync_write fs/read_write.c:491 [inline]
>   vfs_write+0x8ef/0x1490 fs/read_write.c:584
>   ksys_write+0x20f/0x4c0 fs/read_write.c:637
>   __do_sys_write fs/read_write.c:649 [inline]
>   __se_sys_write fs/read_write.c:646 [inline]
>   __x64_sys_write+0x93/0xd0 fs/read_write.c:646
>   do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>   do_syscall_64+0x44/0x110 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x63/0x6b
> 
> Local variable mp_opt created at:
>   subflow_check_req+0x6d/0x15d0 net/mptcp/subflow.c:145
>   subflow_v6_route_req+0x269/0x410 net/mptcp/subflow.c:367
> 
> CPU: 1 PID: 5924 Comm: syz-executor.3 Not tainted 6.7.0-rc8-syzkaller-00055-g5eff55d725a4 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/17/2023
> 
> Fixes: f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 5/5] mptcp: refine opt_mp_capable determination

[Simon Horman]

On Thu, Jan 11, 2024 at 07:49:17PM +0000, Eric Dumazet wrote:
> OPTIONS_MPTCP_MPC is a combination of three flags.
> 
> It would be better to be strict about testing what
> flag is expected, at least for code readability.
> 
> mptcp_parse_option() already makes the distinction.
> 
> - subflow_check_req() should use OPTION_MPTCP_MPC_SYN.
> 
> - mptcp_subflow_init_cookie_req() should use OPTION_MPTCP_MPC_ACK.
> 
> - subflow_finish_connect() should use OPTION_MPTCP_MPC_SYNACK
> 
> - subflow_syn_recv_sock should use OPTION_MPTCP_MPC_ACK
> 
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net 0/5] mptcp: better validation of MPTCPOPT_MP_JOIN option

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 11 Jan 2024 19:49:12 +0000 you wrote:
> Based on a syzbot report (see 4th patch in the series).
> 
> We need to be more explicit about which one of the
> following flag is set by mptcp_parse_option():
> 
> - OPTION_MPTCP_MPJ_SYN
> - OPTION_MPTCP_MPJ_SYNACK
> - OPTION_MPTCP_MPJ_ACK
> 
> [...]

Here is the summary with links:
  - [net,1/5] mptcp: mptcp_parse_option() fix for MPTCPOPT_MP_JOIN
    https://git.kernel.org/netdev/net/c/89e23277f9c1
  - [net,2/5] mptcp: strict validation before using mp_opt->hmac
    https://git.kernel.org/netdev/net/c/c1665273bdc7
  - [net,3/5] mptcp: use OPTION_MPTCP_MPJ_SYNACK in subflow_finish_connect()
    https://git.kernel.org/netdev/net/c/be1d9d9d38da
  - [net,4/5] mptcp: use OPTION_MPTCP_MPJ_SYN in subflow_check_req()
    https://git.kernel.org/netdev/net/c/66ff70df1a91
  - [net,5/5] mptcp: refine opt_mp_capable determination
    https://git.kernel.org/netdev/net/c/724b00c12957

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html