[PATCH mptcp-net 0/2] mptcp: tentative fix for issues246 &&

[PATCH mptcp-net 0/2] mptcp: tentative fix for issues246 &&

[Paolo Abeni]

Sharing these very early, because I can't reproduce the issues locally.
Even the commit messages are early draft.

@Christoph could you please check the relevant repros here?

I *think* there still some refcount problem to address in the new code,
any sharp eyes more then welcome ;)

Targeting -net even if bases on top of corrent mptcp-next, as
the merge window is almost imminent.

Paolo Abeni (2):
  mptcp: use the workqueue to destroy unaccepted sockets.
  mptcp: fix UaF in listener shutdown

 net/mptcp/protocol.c | 27 ++++++++-----
 net/mptcp/protocol.h |  4 +-
 net/mptcp/subflow.c  | 91 +++++---------------------------------------
 3 files changed, 28 insertions(+), 94 deletions(-)

-- 
2.39.1

[PATCH mptcp-net 1/2] mptcp: use the workqueue to destroy unaccepted sockets.

[Paolo Abeni]

Christoph reported a UaF while disposing unaccepted MPC
subflow.

We need to properly clean-up all the paired MPTCP-level
resources and be sure to release the msk last, even when
the unaccepted subflow is destroyed by the TCP internals
via inet_child_forget().

We can re-use the existing MPTCP_WORK_CLOSE_SUBFLOW infra,
explicitly checking that for the critical scenario: the
closed subflow is the MPC one, the msk is not accepted and
eventually going through full cleanup.

With such change, __mptcp_destroy_sock() is always called
on msk sockets, even on accepted ones. We don't need anymore
to transiently drop one sk reference at msk clone time.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 26 +++++++++++++++++---------
 net/mptcp/protocol.h |  3 ++-
 net/mptcp/subflow.c  | 11 +++++++++--
 3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 546df81c162f..d7588535bf6d 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2444,9 +2444,10 @@ static unsigned int mptcp_sync_mss(struct sock *sk, u32 pmtu)
 	return 0;
 }
 
-static void __mptcp_close_subflow(struct mptcp_sock *msk)
+static void __mptcp_close_subflow(struct sock *sk)
 {
 	struct mptcp_subflow_context *subflow, *tmp;
+	struct mptcp_sock *msk = mptcp_sk(sk);
 
 	might_sleep();
 
@@ -2460,7 +2461,15 @@ static void __mptcp_close_subflow(struct mptcp_sock *msk)
 		if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
 			continue;
 
-		mptcp_close_ssk((struct sock *)msk, ssk, subflow);
+		mptcp_close_ssk(sk, ssk, subflow);
+	}
+
+	/* if the MPC subflow has been closed before the msk is accepted,
+	 * msk will never be accept-ed, close it now
+	 */
+	if (!msk->first && msk->in_accept_queue) {
+		sock_set_flag(sk, SOCK_DEAD);
+		sk->sk_state = TCP_CLOSE;
 	}
 }
 
@@ -2684,6 +2693,9 @@ static void mptcp_worker(struct work_struct *work)
 	__mptcp_check_send_data_fin(sk);
 	mptcp_check_data_fin(sk);
 
+	if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
+		__mptcp_close_subflow(sk);
+
 	/* There is no point in keeping around an orphaned sk timedout or
 	 * closed, but we need the msk around to reply to incoming DATA_FIN,
 	 * even if it is orphaned and in FIN_WAIT2 state
@@ -2699,9 +2711,6 @@ static void mptcp_worker(struct work_struct *work)
 		}
 	}
 
-	if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
-		__mptcp_close_subflow(msk);
-
 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
 		__mptcp_retrans(sk);
 
@@ -3149,6 +3158,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
 	msk->local_key = subflow_req->local_key;
 	msk->token = subflow_req->token;
 	msk->subflow = NULL;
+	msk->in_accept_queue = 1;
 	WRITE_ONCE(msk->fully_established, false);
 	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
 		WRITE_ONCE(msk->csum_enabled, true);
@@ -3169,8 +3179,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
 	security_inet_csk_clone(nsk, req);
 	bh_unlock_sock(nsk);
 
-	/* keep a single reference */
-	__sock_put(nsk);
+	/* note: the newly allocated socket refcount is 2 now */
 	return nsk;
 }
 
@@ -3226,8 +3235,6 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
 			goto out;
 		}
 
-		/* acquire the 2nd reference for the owning socket */
-		sock_hold(new_mptcp_sock);
 		newsk = new_mptcp_sock;
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEPASSIVEACK);
 	} else {
@@ -3781,6 +3788,7 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
 		struct mptcp_subflow_context *subflow;
 
 		set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);
+		msk->in_accept_queue = 0;
 
 		/* set ssk->sk_socket of accept()ed flows to mptcp socket.
 		 * This is needed so NOSPACE flag can be set from tcp stack.
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3a055438c65e..252f050c96c6 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -294,7 +294,8 @@ struct mptcp_sock {
 	u8		recvmsg_inq:1,
 			cork:1,
 			nodelay:1,
-			fastopening:1;
+			fastopening:1,
+			in_accept_queue:1;
 	int		connect_flags;
 	struct work_struct work;
 	struct sk_buff  *ooo_last_skb;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index f075607adad4..1dff2ea6d6d3 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -693,9 +693,10 @@ static bool subflow_hmac_valid(const struct request_sock *req,
 
 static void mptcp_force_close(struct sock *sk)
 {
-	/* the msk is not yet exposed to user-space */
+	/* the msk is not yet exposed to user-space, and refcount is 2 */
 	inet_sk_state_store(sk, TCP_CLOSE);
 	sk_common_release(sk);
+	sock_put(sk);
 }
 
 static void subflow_ulp_fallback(struct sock *sk,
@@ -1854,7 +1855,6 @@ void mptcp_subflow_queue_clean(struct sock *listener_sk, struct sock *listener_s
 		struct sock *sk = (struct sock *)msk;
 		bool do_cancel_work;
 
-		sock_hold(sk);
 		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
 		next = msk->dl_next;
 		msk->dl_next = NULL;
@@ -1949,6 +1949,13 @@ static void subflow_ulp_release(struct sock *ssk)
 		 * the ctx alive, will be freed by __mptcp_close_ssk()
 		 */
 		release = ctx->disposable;
+
+		/* inet_child_forget() does not call sk_state_change(),
+		 * explicitly trigger the socket close machinery
+		 */
+		if (!release && !test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW,
+						  &mptcp_sk(sk)->flags))
+			schedule_work(&mptcp_sk(sk)->work);
 		sock_put(sk);
 	}
 
-- 
2.39.1

[PATCH mptcp-net 2/2] mptcp: fix UaF in listener shutdown

[Paolo Abeni]

After the previous patch we don't need anymore special-casing
msk listener socket cleanup: the mptcp worker will process each
of the unaccepted msk sockets.

Just drop the now unnecessary code.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c |  1 -
 net/mptcp/protocol.h |  1 -
 net/mptcp/subflow.c  | 80 --------------------------------------------
 3 files changed, 82 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index d7588535bf6d..ae2d715f82b5 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2400,7 +2400,6 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 		/* otherwise tcp will dispose of the ssk and subflow ctx */
 		if (ssk->sk_state == TCP_LISTEN) {
 			tcp_set_state(ssk, TCP_CLOSE);
-			mptcp_subflow_queue_clean(sk, ssk);
 			inet_csk_listen_stop(ssk);
 			mptcp_event_pm_listener(ssk, MPTCP_EVENT_LISTENER_CLOSED);
 		}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 252f050c96c6..ba4d22ffd228 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -631,7 +631,6 @@ void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 		     struct mptcp_subflow_context *subflow);
 void __mptcp_subflow_send_ack(struct sock *ssk);
 void mptcp_subflow_reset(struct sock *ssk);
-void mptcp_subflow_queue_clean(struct sock *sk, struct sock *ssk);
 void mptcp_sock_graft(struct sock *sk, struct socket *parent);
 struct socket *__mptcp_nmpc_socket(struct mptcp_sock *msk);
 bool __mptcp_close(struct sock *sk, long timeout);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 1dff2ea6d6d3..4f397d336811 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1814,86 +1814,6 @@ static void subflow_state_change(struct sock *sk)
 	}
 }
 
-void mptcp_subflow_queue_clean(struct sock *listener_sk, struct sock *listener_ssk)
-{
-	struct request_sock_queue *queue = &inet_csk(listener_ssk)->icsk_accept_queue;
-	struct mptcp_sock *msk, *next, *head = NULL;
-	struct request_sock *req;
-
-	/* build a list of all unaccepted mptcp sockets */
-	spin_lock_bh(&queue->rskq_lock);
-	for (req = queue->rskq_accept_head; req; req = req->dl_next) {
-		struct mptcp_subflow_context *subflow;
-		struct sock *ssk = req->sk;
-		struct mptcp_sock *msk;
-
-		if (!sk_is_mptcp(ssk))
-			continue;
-
-		subflow = mptcp_subflow_ctx(ssk);
-		if (!subflow || !subflow->conn)
-			continue;
-
-		/* skip if already in list */
-		msk = mptcp_sk(subflow->conn);
-		if (msk->dl_next || msk == head)
-			continue;
-
-		msk->dl_next = head;
-		head = msk;
-	}
-	spin_unlock_bh(&queue->rskq_lock);
-	if (!head)
-		return;
-
-	/* can't acquire the msk socket lock under the subflow one,
-	 * or will cause ABBA deadlock
-	 */
-	release_sock(listener_ssk);
-
-	for (msk = head; msk; msk = next) {
-		struct sock *sk = (struct sock *)msk;
-		bool do_cancel_work;
-
-		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
-		next = msk->dl_next;
-		msk->dl_next = NULL;
-
-		/* The upcoming mptcp_close is going to drop all the references
-		 * to the first subflow, ignoring that one of such reference is
-		 * owned by the request socket still in the accept queue and that
-		 * later inet_csk_listen_stop will drop it.
-		 * Acquire an extra reference here to avoid an UaF at that point.
-		 */
-		if (msk->first)
-			sock_hold(msk->first);
-
-		do_cancel_work = __mptcp_close(sk, -1);
-		release_sock(sk);
-		if (do_cancel_work) {
-			/* lockdep will report a false positive ABBA deadlock
-			 * between cancel_work_sync and the listener socket.
-			 * The involved locks belong to different sockets WRT
-			 * the existing AB chain.
-			 * Using a per socket key is problematic as key
-			 * deregistration requires process context and must be
-			 * performed at socket disposal time, in atomic
-			 * context.
-			 * Just tell lockdep to consider the listener socket
-			 * released here.
-			 */
-			mutex_release(&listener_sk->sk_lock.dep_map, _RET_IP_);
-			mptcp_cancel_work(sk);
-			mutex_acquire(&listener_sk->sk_lock.dep_map,
-				      SINGLE_DEPTH_NESTING, 0, _RET_IP_);
-		}
-		sock_put(sk);
-	}
-
-	/* we are still under the listener msk socket lock */
-	lock_sock_nested(listener_ssk, SINGLE_DEPTH_NESTING);
-}
-
 static int subflow_ulp_init(struct sock *sk)
 {
 	struct inet_connection_sock *icsk = inet_csk(sk);
-- 
2.39.1

Re: [PATCH mptcp-net 1/2] mptcp: use the workqueue to destroy unaccepted sockets.

[Paolo Abeni]

On Wed, 2023-02-08 at 22:44 +0100, Paolo Abeni wrote:
> Christoph reported a UaF while disposing unaccepted MPC
> subflow.
> 
> We need to properly clean-up all the paired MPTCP-level
> resources and be sure to release the msk last, even when
> the unaccepted subflow is destroyed by the TCP internals
> via inet_child_forget().
> 
> We can re-use the existing MPTCP_WORK_CLOSE_SUBFLOW infra,
> explicitly checking that for the critical scenario: the
> closed subflow is the MPC one, the msk is not accepted and
> eventually going through full cleanup.
> 
> With such change, __mptcp_destroy_sock() is always called
> on msk sockets, even on accepted ones. We don't need anymore
> to transiently drop one sk reference at msk clone time.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  net/mptcp/protocol.c | 26 +++++++++++++++++---------
>  net/mptcp/protocol.h |  3 ++-
>  net/mptcp/subflow.c  | 11 +++++++++--
>  3 files changed, 28 insertions(+), 12 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 546df81c162f..d7588535bf6d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2444,9 +2444,10 @@ static unsigned int mptcp_sync_mss(struct sock *sk, u32 pmtu)
>  	return 0;
>  }
>  
> -static void __mptcp_close_subflow(struct mptcp_sock *msk)
> +static void __mptcp_close_subflow(struct sock *sk)
>  {
>  	struct mptcp_subflow_context *subflow, *tmp;
> +	struct mptcp_sock *msk = mptcp_sk(sk);
>  
>  	might_sleep();
>  
> @@ -2460,7 +2461,15 @@ static void __mptcp_close_subflow(struct mptcp_sock *msk)
>  		if (!skb_queue_empty_lockless(&ssk->sk_receive_queue))
>  			continue;
>  
> -		mptcp_close_ssk((struct sock *)msk, ssk, subflow);
> +		mptcp_close_ssk(sk, ssk, subflow);
> +	}
> +
> +	/* if the MPC subflow has been closed before the msk is accepted,
> +	 * msk will never be accept-ed, close it now
> +	 */
> +	if (!msk->first && msk->in_accept_queue) {
> +		sock_set_flag(sk, SOCK_DEAD);
> +		sk->sk_state = TCP_CLOSE;
>  	}
>  }
>  
> @@ -2684,6 +2693,9 @@ static void mptcp_worker(struct work_struct *work)
>  	__mptcp_check_send_data_fin(sk);
>  	mptcp_check_data_fin(sk);
>  
> +	if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
> +		__mptcp_close_subflow(sk);
> +
>  	/* There is no point in keeping around an orphaned sk timedout or
>  	 * closed, but we need the msk around to reply to incoming DATA_FIN,
>  	 * even if it is orphaned and in FIN_WAIT2 state
> @@ -2699,9 +2711,6 @@ static void mptcp_worker(struct work_struct *work)
>  		}
>  	}
>  
> -	if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
> -		__mptcp_close_subflow(msk);
> -
>  	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
>  		__mptcp_retrans(sk);
>  
> @@ -3149,6 +3158,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
>  	msk->local_key = subflow_req->local_key;
>  	msk->token = subflow_req->token;
>  	msk->subflow = NULL;
> +	msk->in_accept_queue = 1;
>  	WRITE_ONCE(msk->fully_established, false);
>  	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
>  		WRITE_ONCE(msk->csum_enabled, true);
> @@ -3169,8 +3179,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
>  	security_inet_csk_clone(nsk, req);
>  	bh_unlock_sock(nsk);
>  
> -	/* keep a single reference */
> -	__sock_put(nsk);
> +	/* note: the newly allocated socket refcount is 2 now */
>  	return nsk;
>  }
>  
> @@ -3226,8 +3235,6 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
>  			goto out;
>  		}
>  
> -		/* acquire the 2nd reference for the owning socket */
> -		sock_hold(new_mptcp_sock);
>  		newsk = new_mptcp_sock;
>  		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPCAPABLEPASSIVEACK);
>  	} else {
> @@ -3781,6 +3788,7 @@ static int mptcp_stream_accept(struct socket *sock, struct socket *newsock,
>  		struct mptcp_subflow_context *subflow;
>  
>  		set_bit(SOCK_CUSTOM_SOCKOPT, &newsock->flags);
> +		msk->in_accept_queue = 0;
>  
>  		/* set ssk->sk_socket of accept()ed flows to mptcp socket.
>  		 * This is needed so NOSPACE flag can be set from tcp stack.
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 3a055438c65e..252f050c96c6 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -294,7 +294,8 @@ struct mptcp_sock {
>  	u8		recvmsg_inq:1,
>  			cork:1,
>  			nodelay:1,
> -			fastopening:1;
> +			fastopening:1,
> +			in_accept_queue:1;
>  	int		connect_flags;
>  	struct work_struct work;
>  	struct sk_buff  *ooo_last_skb;
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index f075607adad4..1dff2ea6d6d3 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -693,9 +693,10 @@ static bool subflow_hmac_valid(const struct request_sock *req,
>  
>  static void mptcp_force_close(struct sock *sk)
>  {
> -	/* the msk is not yet exposed to user-space */
> +	/* the msk is not yet exposed to user-space, and refcount is 2 */
>  	inet_sk_state_store(sk, TCP_CLOSE);
>  	sk_common_release(sk);
> +	sock_put(sk);
>  }
>  
>  static void subflow_ulp_fallback(struct sock *sk,
> @@ -1854,7 +1855,6 @@ void mptcp_subflow_queue_clean(struct sock *listener_sk, struct sock *listener_s
>  		struct sock *sk = (struct sock *)msk;
>  		bool do_cancel_work;
>  
> -		sock_hold(sk);
>  		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
>  		next = msk->dl_next;
>  		msk->dl_next = NULL;
> @@ -1949,6 +1949,13 @@ static void subflow_ulp_release(struct sock *ssk)
>  		 * the ctx alive, will be freed by __mptcp_close_ssk()
>  		 */
>  		release = ctx->disposable;
> +
> +		/* inet_child_forget() does not call sk_state_change(),
> +		 * explicitly trigger the socket close machinery
> +		 */
> +		if (!release && !test_and_set_bit(MPTCP_WORK_CLOSE_SUBFLOW,
> +						  &mptcp_sk(sk)->flags))
> +			schedule_work(&mptcp_sk(sk)->work);

The above is buggy. Instead of 
			schedule_work(&mptcp_sk(sk)->work);
should be
			mptcp_schedule_work(sk);

@Christoph: could you add the above editing while testing, or do you
prefer I'll send a the full series edited?

Thanks!

Paolo

Re: mptcp: fix UaF in listener shutdown: Tests Results

[MPTCP CI]

Hi Paolo,

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):
  - Critical: 1 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/4517118641700864
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4517118641700864/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): packetdrill_sockopts - Critical: 1 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/5080068595122176
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5080068595122176/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6205968501964800
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6205968501964800/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5643018548543488
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5643018548543488/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/ae7c0cbcdb5b


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-debug

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 (Tessares)

Re: [PATCH mptcp-net 0/2] mptcp: tentative fix for issues246 &&

[Christoph Paasch]

> On Feb 8, 2023, at 1:44 PM, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> Sharing these very early, because I can't reproduce the issues locally.
> Even the commit messages are early draft.

Turns out I can’t repro either (neither with the syzkaller-repro nor with the c-repro). All I have is the syzkaller report…

I will update my syzkaller kernel and also apply Kuniyuki’s patch and run syzkaller again.


Christoph

> 
> @Christoph could you please check the relevant repros here?
> 
> I *think* there still some refcount problem to address in the new code,
> any sharp eyes more then welcome ;)
> 
> Targeting -net even if bases on top of corrent mptcp-next, as
> the merge window is almost imminent.
> 
> Paolo Abeni (2):
>  mptcp: use the workqueue to destroy unaccepted sockets.
>  mptcp: fix UaF in listener shutdown
> 
> net/mptcp/protocol.c | 27 ++++++++-----
> net/mptcp/protocol.h |  4 +-
> net/mptcp/subflow.c  | 91 +++++---------------------------------------
> 3 files changed, 28 insertions(+), 94 deletions(-)
> 
> -- 
> 2.39.1
> 
> 

Re: [PATCH mptcp-net 0/2] mptcp: tentative fix for issues246 &&

[Christoph Paasch]

> On Feb 8, 2023, at 1:44 PM, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> Sharing these very early, because I can't reproduce the issues locally.
> Even the commit messages are early draft.
> 
> @Christoph could you please check the relevant repros here?
> 
> I *think* there still some refcount problem to address in the new code,
> any sharp eyes more then welcome ;)
> 
> Targeting -net even if bases on top of corrent mptcp-next, as
> the merge window is almost imminent.
> 
> Paolo Abeni (2):
>  mptcp: use the workqueue to destroy unaccepted sockets.
>  mptcp: fix UaF in listener shutdown
> 
> net/mptcp/protocol.c | 27 ++++++++-----
> net/mptcp/protocol.h |  4 +-
> net/mptcp/subflow.c  | 91 +++++---------------------------------------
> 3 files changed, 28 insertions(+), 94 deletions(-)

I was able to get a new reproducer for a UaF read in subflow_error_report.

Your patches (with the schedule-work fix) indeed fix the KASAN-issue.

Tested-by: Christoph Paasch <cpaasch@apple.com>


Christoph

Re: mptcp: fix UaF in listener shutdown: Tests Results

[MPTCP CI]

Hi Paolo,

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):
  - Critical: 1 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/5329004899598336
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5329004899598336/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Critical: 1 Call Trace(s) ❌:
  - Task: https://cirrus-ci.com/task/5047529922887680
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5047529922887680/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6454904806440960
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6454904806440960/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6173429829730304
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6173429829730304/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/cbc3e5c367d4


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-debug

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 (Tessares)