netdev - RE: [PATCH net 4/4] tls: Fix tls

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]

Message-ID: <DB7PR04MB4252D936A2C713C60542A6828B740@DB7PR04MB4252.eurprd04.prod.outlook.com>
Date:   Wed, 27 Feb 2019 03:08:50 +0000
From:   Vakul Garg <vakul.garg@....com>
To:     Dave Watson <davejwatson@...com>,
        Boris Pismenny <borisp@...lanox.com>
CC:     "aviadye@...lanox.com" <aviadye@...lanox.com>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "eranbe@...lanox.com" <eranbe@...lanox.com>
Subject: RE: [PATCH net 4/4] tls: Fix tls_device receive



> -----Original Message-----
> From: Dave Watson <davejwatson@...com>
> Sent: Wednesday, February 27, 2019 2:05 AM
> To: Boris Pismenny <borisp@...lanox.com>
> Cc: aviadye@...lanox.com; john.fastabend@...il.com;
> daniel@...earbox.net; Vakul Garg <vakul.garg@....com>;
> netdev@...r.kernel.org; eranbe@...lanox.com
> Subject: Re: [PATCH net 4/4] tls: Fix tls_device receive
> 
> On 02/26/19 02:12 PM, Boris Pismenny wrote:
> > Currently, the receive function fails to handle records already
> > decrypted by the device due to the commit mentioned below.
> >
> > This commit advances the TLS record sequence number and prepares the
> > context to handle the next record.
> >
> > Fixes: fedf201e1296 ("net: tls: Refactor control message handling on
> > recv")
> > Signed-off-by: Boris Pismenny <borisp@...lanox.com>
> > Reviewed-by: Eran Ben Elisha <eranbe@...lanox.com>
> > ---
> >  net/tls/tls_sw.c | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index
> > f515cd7e984e..85da10182d8d 100644
> > --- a/net/tls/tls_sw.c
> > +++ b/net/tls/tls_sw.c
> > @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk,
> > struct sk_buff *skb,
> >
> >  			return err;
> >  		}
> > -
> > -		rxm->full_len -= padding_length(ctx, tls_ctx, skb);
> > -
> > -		rxm->offset += prot->prepend_size;
> > -		rxm->full_len -= prot->overhead_size;
> > -		tls_advance_record_sn(sk, &tls_ctx->rx, version);
> > -		ctx->decrypted = true;
> > -		ctx->saved_data_ready(sk);
> >  	} else {
> >  		*zc = false;
> >  	}
> >
> > +	rxm->full_len -= padding_length(ctx, tls_ctx, skb);
> > +	rxm->offset += prot->prepend_size;
> > +	rxm->full_len -= prot->overhead_size;
> > +	tls_advance_record_sn(sk, &tls_ctx->rx, version);
> > +	ctx->decrypted = true;
> > +	ctx->saved_data_ready(sk);
> > +
> >  	return err;
> >  }
> 
> This breaks the tls.control_msg test:
> 
>   [ RUN      ] tls.control_msg
>   tls.c:764:tls.control_msg:Expected memcmp(buf, test_str, send_len)
> (18446744073709551614) == 0 (0)
>   tls.c:777:tls.control_msg:Expected memcmp(buf, test_str, send_len)
> (18446744073709551614) == 0 (0)
>   tls.control_msg: Test failed at step #8
> 
> So either control message handling needs to only call decrypt_skb_update
> once, or we need a new flag or something to handle the device case

I prefer to remove variable 'decrypted' in context.
This is no longer required as we already have an rx_list in context for storing decrypted records.
So for any record which we decrypted but did not return to user space 
(e.g. for the case when user used recv() and it lead to decryption of non-data record), we should
it in rx_list.