netdev - RE: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without skb_cow

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

Message-ID: <DB7PR04MB42528E2A7F6249CC15ED502B8B270@DB7PR04MB4252.eurprd04.prod.outlook.com>
Date:   Tue, 7 Aug 2018 04:41:59 +0000
From:   Vakul Garg <vakul.garg@....com>
To:     Doron Roberts-Kedes <doronrk@...com>
CC:     "David S . Miller" <davem@...emloft.net>,
        Dave Watson <davejwatson@...com>,
        Boris Pismenny <borisp@...lanox.com>,
        Aviad Yehezkel <aviadye@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH net-next] net/tls: Calculate nsg for zerocopy path without
 skb_cow_data.



> -----Original Message-----
> From: Doron Roberts-Kedes [mailto:doronrk@...com]
> Sent: Tuesday, August 7, 2018 12:02 AM
> To: Vakul Garg <vakul.garg@....com>
> Cc: David S . Miller <davem@...emloft.net>; Dave Watson
> <davejwatson@...com>; Boris Pismenny <borisp@...lanox.com>; Aviad
> Yehezkel <aviadye@...lanox.com>; netdev@...r.kernel.org
> Subject: Re: [PATCH net-next] net/tls: Calculate nsg for zerocopy path
> without skb_cow_data.
> 
> On Fri, Aug 03, 2018 at 11:49:02AM -0700, Doron Roberts-Kedes wrote:
> > On Fri, Aug 03, 2018 at 01:23:33AM +0000, Vakul Garg wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Doron Roberts-Kedes [mailto:doronrk@...com]
> > > > Sent: Friday, August 3, 2018 6:00 AM
> > > > To: David S . Miller <davem@...emloft.net>
> > > > Cc: Dave Watson <davejwatson@...com>; Vakul Garg
> > > > <vakul.garg@....com>; Boris Pismenny <borisp@...lanox.com>;
> Aviad
> > > > Yehezkel <aviadye@...lanox.com>; netdev@...r.kernel.org; Doron
> > > > Roberts-Kedes <doronrk@...com>
> > > > Subject: [PATCH net-next] net/tls: Calculate nsg for zerocopy path
> > > > without skb_cow_data.
> > > >
> > > > decrypt_skb fails if the number of sg elements required to map is
> > > > greater than MAX_SKB_FRAGS. As noted by Vakul Garg, nsg must
> > > > always be calculated, but skb_cow_data adds unnecessary memcpy's
> > > > for the zerocopy case.
> > > >
> > > > The new function skb_nsg calculates the number of scatterlist
> > > > elements required to map the skb without the extra overhead of
> > > > skb_cow_data. This function mimics the structure of skb_to_sgvec.
> > > >
> > > > Fixes: c46234ebb4d1 ("tls: RX path for ktls")
> > > > Signed-off-by: Doron Roberts-Kedes <doronrk@...com>
> > > > ---
> > > >  net/tls/tls_sw.c | 89
> > > > ++++++++++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 86 insertions(+), 3 deletions(-)
> > > >
> > > >  	memcpy(iv, tls_ctx->rx.iv, TLS_CIPHER_AES_GCM_128_SALT_SIZE);
> > > >  	if (!sgout) {
> > > > -		nsg = skb_cow_data(skb, 0, &unused) + 1;
> > > > +		nsg = skb_cow_data(skb, 0, &unused);
> > > > +	} else {
> > > > +		nsg = skb_nsg(skb,
> > > > +			      rxm->offset + tls_ctx->rx.prepend_size,
> > > > +			      rxm->full_len - tls_ctx->rx.prepend_size);
> > > > +		if (nsg <= 0)
> > > > +			return nsg;
> > > Comparison should be (nsg < 1). TLS forbids '0' sized records.
> >
> > Yes true, v2 incoming
> >
> 
> Glancing at this a second time, I actually don't believe this should be
> changed. nsg <= 0 is equivalent to nsg < 1. 

Correct.


> Returning 0 if the record is
> 0 sized is the proper behavior here, since decrypting a zero-length skb is a no-
> op. It is true that zero sized TLS records are forbidden, but it is confusing to
> enforce that in this part of the code. I would be surpised to learn that
> tls_sw_recvmsg could be invoked with a len equal to 0, but if I'm wrong, and
> that case does need to be handled, then it should be in a different patch.