netdev - RE: [Crypto v7 03/12] tls: support for inline 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] [day] [month] [year] [list]

Date:   Sat, 24 Feb 2018 11:34:04 +0000
From:   Atul Gupta <atul.gupta@...lsio.com>
To:     Dave Watson <davejwatson@...com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "herbert@...dor.apana.org.au" <herbert@...dor.apana.org.au>,
        "sd@...asysnail.net" <sd@...asysnail.net>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ganesh GR <ganeshgr@...lsio.com>
Subject: RE: [Crypto v7 03/12] tls: support for inline tls



-----Original Message-----
From: Dave Watson [mailto:davejwatson@...com] 
Sent: Friday, February 23, 2018 11:03 PM
To: Atul Gupta <atul.gupta@...lsio.com>
Cc: davem@...emloft.net; herbert@...dor.apana.org.au; sd@...asysnail.net; linux-crypto@...r.kernel.org; netdev@...r.kernel.org; Ganesh GR <ganeshgr@...lsio.com>
Subject: Re: [Crypto v7 03/12] tls: support for inline tls

On 02/23/18 04:58 PM, Atul Gupta wrote:
> > On 02/22/18 11:21 PM, Atul Gupta wrote:
> > > @@ -403,6 +431,15 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
> > >  		goto err_crypto_info;
> > >  	}
> > >  
> > > +	rc = tls_offload_dev_absent(sk);
> > > +	if (rc == -EINVAL) {
> > > +		goto out;
> > > +	} else if (rc == -EEXIST) {
> > > +		/* Retain HW unhash for cleanup and move to SW Tx */
> > > +		sk->sk_prot[TLS_BASE_TX].unhash =
> > > +			sk->sk_prot[TLS_FULL_HW].unhash;
> > 
> > I'm still confused by this, it lookes like it is modifying the global tls_prots without taking a lock?  And modifying it for all sockets, not just this one?  One way to fix might be to always set an unhash in TLS_BASE_TX, and then have a function pointer unhash in ctx.
> 
> code enters do_tls_setsockopt_tx only for those offload capable dev which does not define FULL_HW setsockopt as done by chtls, unhash prot update is required for cleanup/revert of setup done in tls_hw_hash. This update does not impact SW or other Inline HW path. 

I still don't follow.  If it doesn't impact SW, then what is it doing?
According to the comment, we're moving to SW tx, where sk_prot will be &tls_prot[TLS_SW_TX], and the unhash function you set here in TLS_BASE_TX won't be called.

some of the scenarios I originally thought:
- tls_init finds the Inline offload dev and sets the TLS_FULL_HW but setsockopt remains do_tls_setsockopt_tx, In the above path we continue in TLS_SW_TX mode with updated unhash. Since, sw_tx prot is borrowed from base_tx we modified the base_tx prot unhash for cleanup.
- tls_offload_dev_absent finds no device i.e rc=0. Continue in TLS_SW_TX mode. No change required.
- Inline tls device is added after tls_init is called, do_tls_setsockopt_tx will see tls_offload_dev_absent return EEXIST and will modify unhash but only if tx_conf = TLS_FULL_HW [missing now] and you rightly pointed that it ends up modifying base prot for all sk which is not we want. My worry was losing hw specific unhash.
I see calling tls_offload_dev_absent in do_tls_setsockopt_tx an overkill, the sk here perhaps require no update to continue in SW_TX, the HW unhash is still assigned to tls_init 'sk' for cleanup in the close path, better to remove tls_offload_dev_absent altogether and simplify.