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

Message-ID: <20180806183219.GA78698@doronrk-mbp.dhcp.thefacebook.com>
Date:   Mon, 6 Aug 2018 11:32:19 -0700
From:   Doron Roberts-Kedes <doronrk@...com>
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" <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. 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.