netdev - Re: [PATCH net-next 2/2] mptcp: add reset reason options in some places

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 05 Apr 2024 10:16:07 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jason Xing <kerneljasonxing@...il.com>, davem@...emloft.net, 
 edumazet@...gle.com, kuba@...nel.org, matttbe@...nel.org,
 martineau@...nel.org,  geliang@...nel.org
Cc: mptcp@...ts.linux.dev, netdev@...r.kernel.org, Jason Xing
	 <kernelxing@...cent.com>
Subject: Re: [PATCH net-next 2/2] mptcp: add reset reason options in some
 places

On Fri, 2024-04-05 at 10:39 +0800, Jason Xing wrote:
> From: Jason Xing <kernelxing@...cent.com>
> 
> The reason codes are handled in two ways nowadays (quoting Mat Martineau):
> 1. Sending in the MPTCP option on RST packets when there is no subflow
> context available (these use subflow_add_reset_reason() and directly call
> a TCP-level send_reset function)
> 2. The "normal" way via subflow->reset_reason. This will propagate to both
> the outgoing reset packet and to a local path manager process via netlink
> in mptcp_event_sub_closed()
> 
> RFC 8684 defines the skb reset reason behaviour which is not required
> even though in some places:
> 
>     A host sends a TCP RST in order to close a subflow or reject
>     an attempt to open a subflow (MP_JOIN). In order to let the
>     receiving host know why a subflow is being closed or rejected,
>     the TCP RST packet MAY include the MP_TCPRST option (Figure 15).
>     The host MAY use this information to decide, for example, whether
>     it tries to re-establish the subflow immediately, later, or never.
> 
> Since the commit dc87efdb1a5cd ("mptcp: add mptcp reset option support")
> introduced this feature about three years ago, we can fully use it.
> There remains some places where we could insert reason into skb as
> we can see in this patch.
> 
> Many thanks to Mat for help:)
> 
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
> ---
>  net/mptcp/subflow.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 1626dd20c68f..49f746d91884 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -301,8 +301,13 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
>  		return dst;
>  
>  	dst_release(dst);
> -	if (!req->syncookie)
> +	if (!req->syncookie) {
> +		struct mptcp_ext *mpext = mptcp_get_ext(skb);
> +
> +		if (mpext)
> +			subflow_add_reset_reason(skb, mpext->reset_reason);

uhm? subflow_add_reset_reason() will do:

	mptcp_ext_add(skb)->reset_reason = mpext->reset_reason

The above looks like a no-op. 

Possibly we should instead ensure that subflow_check_req() calls
subflow_add_reset_reason() with reasonable arguments on all the error
paths?!?

Something alike the (completely untested) following

Cheers,

Paolo
---
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 6042a47da61b..298c6342a78c 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -150,8 +150,10 @@ static int subflow_check_req(struct request_sock *req,
 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
 	 * TCP option space.
 	 */
-	if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info))
+	if (rcu_access_pointer(tcp_sk(sk_listener)->md5sig_info)) {
+		subflow_add_reset_reason(skb, MPTCP_RST_EMPTCP);
 		return -EINVAL;
+	}
 #endif
 
 	mptcp_get_options(skb, &mp_opt);
@@ -219,6 +221,7 @@ static int subflow_check_req(struct request_sock *req,
 				 ntohs(inet_sk((struct sock *)subflow_req->msk)->inet_sport));
 			if (!mptcp_pm_sport_in_anno_list(subflow_req->msk, sk_listener)) {
 				SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MISMATCHPORTSYNRX);
+				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
 				return -EPERM;
 			}
 			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINPORTSYNRX);
@@ -227,10 +230,12 @@ static int subflow_check_req(struct request_sock *req,
 		subflow_req_create_thmac(subflow_req);
 
 		if (unlikely(req->syncookie)) {
-			if (mptcp_can_accept_new_subflow(subflow_req->msk))
-				subflow_init_req_cookie_join_save(subflow_req, skb);
-			else
+			if (!mptcp_can_accept_new_subflow(subflow_req->msk)) {
+				subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT);
 				return -EPERM;
+			}
+
+			subflow_init_req_cookie_join_save(subflow_req, skb);
 		}
 
 		pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token,