netdev - Re: [PATCH net 3/3] mptcp: drop req socket remote

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5f0a72a715fdfcd64119f0ad95b076570959fdf5.camel@redhat.com>
Date:   Tue, 21 Apr 2020 10:24:52 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Mat Martineau <mathew.j.martineau@...ux.intel.com>
Cc:     netdev@...r.kernel.org,
        Matthieu Baerts <matthieu.baerts@...sares.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Christoph Paasch <cpaasch@...le.com>,
        Florian Westphal <fw@...len.de>
Subject: Re: [PATCH net 3/3] mptcp: drop req socket remote_key* fields

On Mon, 2020-04-20 at 13:05 -0700, Mat Martineau wrote:
> On Mon, 20 Apr 2020, Paolo Abeni wrote:
> 
> > We don't need them, as we can use the current ingress opt
> > data instead. Setting them in syn_recv_sock() may causes
> > inconsistent mptcp socket status, as per previous commit.
> > 
> > Fixes: cc7972ea1932 ("mptcp: parse and emit MP_CAPABLE option according to v1 spec")
> > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > ---
> > net/mptcp/protocol.c |  8 +++++---
> > net/mptcp/protocol.h |  8 ++++----
> > net/mptcp/subflow.c  | 20 ++++++++++----------
> > 3 files changed, 19 insertions(+), 17 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index d275c1e827fe..58ad03fc1bbc 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -1345,7 +1345,9 @@ static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk)
> > }
> > #endif
> > 
> > -struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
> > +struct sock *mptcp_sk_clone(const struct sock *sk,
> > +			    const struct tcp_options_received *opt_rx,
> > +			    struct request_sock *req)
> > {
> > 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
> > 	struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);
> > @@ -1383,9 +1385,9 @@ struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
> > 
> > 	msk->write_seq = subflow_req->idsn + 1;
> > 	atomic64_set(&msk->snd_una, msk->write_seq);
> > -	if (subflow_req->remote_key_valid) {
> > +	if (opt_rx->mptcp.mp_capable) {
> > 		msk->can_ack = true;
> > -		msk->remote_key = subflow_req->remote_key;
> > +		msk->remote_key = opt_rx->mptcp.sndr_key;
> > 		mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
> > 		ack_seq++;
> > 		msk->ack_seq = ack_seq;
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 67448002a2d7..a2b3048037d0 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -206,12 +206,10 @@ struct mptcp_subflow_request_sock {
> > 	struct	tcp_request_sock sk;
> > 	u16	mp_capable : 1,
> > 		mp_join : 1,
> > -		backup : 1,
> > -		remote_key_valid : 1;
> > +		backup : 1;
> > 	u8	local_id;
> > 	u8	remote_id;
> > 	u64	local_key;
> > -	u64	remote_key;
> > 	u64	idsn;
> > 	u32	token;
> > 	u32	ssn_offset;
> > @@ -332,7 +330,9 @@ void mptcp_proto_init(void);
> > int mptcp_proto_v6_init(void);
> > #endif
> > 
> > -struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req);
> > +struct sock *mptcp_sk_clone(const struct sock *sk,
> > +			    const struct tcp_options_received *opt_rx,
> > +			    struct request_sock *req);
> > void mptcp_get_options(const struct sk_buff *skb,
> > 		       struct tcp_options_received *opt_rx);
> > 
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 10090ca3d3e0..87c094702d63 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -133,7 +133,6 @@ static void subflow_init_req(struct request_sock *req,
> > 
> > 	subflow_req->mp_capable = 0;
> > 	subflow_req->mp_join = 0;
> > -	subflow_req->remote_key_valid = 0;
> > 
> > #ifdef CONFIG_TCP_MD5SIG
> > 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of
> > @@ -404,6 +403,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> > 
> > 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
> > 
> > +	opt_rx.mptcp.mp_capable = 0;
> > 	if (tcp_rsk(req)->is_mptcp == 0)
> > 		goto create_child;
> > 
> > @@ -418,18 +418,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> > 			goto create_msk;
> > 		}
> > 
> > -		opt_rx.mptcp.mp_capable = 0;
> > 		mptcp_get_options(skb, &opt_rx);
> > -		if (opt_rx.mptcp.mp_capable) {
> > -			subflow_req->remote_key = opt_rx.mptcp.sndr_key;
> > -			subflow_req->remote_key_valid = 1;
> > -		} else {
> > +		if (!opt_rx.mptcp.mp_capable) {
> > 			fallback = true;
> > 			goto create_child;
> > 		}
> > 
> > create_msk:
> > -		new_msk = mptcp_sk_clone(listener->conn, req);
> > +		new_msk = mptcp_sk_clone(listener->conn, &opt_rx, req);
> > 		if (!new_msk)
> > 			fallback = true;
> > 	} else if (subflow_req->mp_join) {
> > @@ -473,6 +469,13 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
> > 			mptcp_pm_new_connection(mptcp_sk(new_msk), 1);
> > 			ctx->conn = new_msk;
> > 			new_msk = NULL;
> > +
> > +			/* with OoO packets we can reach here without ingress
> > +			 * mpc option
> > +			 */
> > +			ctx->remote_key = opt_rx.mptcp.sndr_key;
> > +			ctx->fully_established = opt_rx.mptcp.mp_capable;
> > +			ctx->can_ack = opt_rx.mptcp.mp_capable;
> 
> If this code is reached without an incoming mpc option, it looks like 
> ctx->remote_key gets junk off the stack. Maybe this instead?

The idea here is to avoid additional conditional. The remote_key will
be used only after 'can_ack' becomes true, and the '!can_ack' condition
here is unlikely. Overall this should be faster and safe.

Thanks for checking,

Paolo