netdev - Re: [PATCH net] mptcp: always include dack if possible.

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] [day] [month] [year] [list]

Date:   Tue, 03 Mar 2020 22:14:45 +0100
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>,
        "David S. Miller" <davem@...emloft.net>,
        Christoph Paasch <cpaasch@...le.com>
Subject: Re: [PATCH net] mptcp: always include dack if possible.

On Tue, 2020-03-03 at 10:58 -0800, Mat Martineau wrote:
> On Tue, 3 Mar 2020, Paolo Abeni wrote:
> 
> > Currently passive MPTCP socket can skip including the DACK
> > option - if the peer sends data before accept() completes.
> > 
> > The above happens because the msk 'can_ack' flag is set
> > only after the accept() call.
> > 
> > Such missing DACK option may cause - as per RFC spec -
> > unwanted fallback to TCP.
> > 
> > This change addresses the issue using the key material
> > available in the current subflow, if any, to create a suitable
> > dack option when msk ack seq is not yet available.
> > 
> > Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option")
> > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > ---
> > net/mptcp/options.c | 17 +++++++++++++++--
> > 1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 45acd877bef3..9eb84115dc35 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -334,6 +334,8 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > 	struct mptcp_sock *msk;
> > 	unsigned int ack_size;
> > 	bool ret = false;
> > +	bool can_ack;
> > +	u64 ack_seq;
> > 	u8 tcp_fin;
> > 
> > 	if (skb) {
> > @@ -360,9 +362,20 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > 		ret = true;
> > 	}
> > 
> > +	/* passive sockets msk will set the 'can_ack' after accept(), even
> > +	 * if the first subflow may have the already the remote key handy
> > +	 */
> > +	can_ack = true;
> > 	opts->ext_copy.use_ack = 0;
> > 	msk = mptcp_sk(subflow->conn);
> > -	if (!msk || !READ_ONCE(msk->can_ack)) {
> > +	if (likely(msk && READ_ONCE(msk->can_ack)))
> > +		ack_seq = msk->ack_seq;
> > +	else if (subflow->can_ack)
> > +		mptcp_crypto_key_sha(subflow->remote_key, NULL, &ack_seq);
> 
> The other code paths that set the initial sequence number all increment it 
> before sending (to ack SYN+MP_CAPABLE). It looks like the spec allows the 
> value calculated here, but we might as well be consistent about the 
> initial value we send over the wire.

Thanks for the feedback! Agreed. I'll send a v2 tomorrow.

Cheers,

Paolo