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

Message-ID: <20180223162313.GA82586@davejwatson-mba>
Date:   Fri, 23 Feb 2018 08:23:13 -0800
From:   Dave Watson <davejwatson@...com>
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>, <ganeshgr@...lsio.com>
Subject: Re: [Crypto v7 03/12] tls: support for inline tls

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.

> +static void tls_hw_unhash(struct sock *sk)
> +{
> +	struct tls_device *dev;
> +
> +	mutex_lock(&device_mutex);
> +	list_for_each_entry(dev, &device_list, dev_list) {
> +		if (dev->unhash)
> +			dev->unhash(dev, sk);
> +	}
> +	mutex_unlock(&device_mutex);
> +	sk->sk_prot->unhash(sk);

I would have thought unhash() here was tls_hw_unhash, doesn't the
original callback need to be saved like the other ones
(set/getsockopt, etc) in tls_init?  Similar for hash().

It looks like in patch 11 you directly call tcp_prot.hash/unhash, so
it doesn't have this issue.