netdev - Re: [PATCH net-next 3/5] tls: rx: add sockopt for enabling optimistic decrypt with TLS 1.3

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: <b111828e6ac34baad9f4e783127eba8344ac252d.camel@nvidia.com>
Date:   Fri, 8 Jul 2022 14:14:44 +0000
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     "kuba@...nel.org" <kuba@...nel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        Boris Pismenny <borisp@...dia.com>
Subject: Re: [PATCH net-next 3/5] tls: rx: add sockopt for enabling optimistic
 decrypt with TLS 1.3

On Tue, 2022-07-05 at 16:59 -0700, Jakub Kicinski wrote:
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index 2ffede463e4a..1b3efc96db0b 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -533,6 +533,37 @@ static int do_tls_getsockopt_tx_zc(struct sock *sk, char __user *optval,
>  	return 0;
>  }
>  
> +static int do_tls_getsockopt_no_pad(struct sock *sk, char __user *optval,
> +				    int __user *optlen)
> +{
> +	struct tls_context *ctx = tls_get_ctx(sk);
> +	unsigned int value;
> +	int err, len;
> +
> +	if (ctx->prot_info.version != TLS_1_3_VERSION)
> +		return -EINVAL;
> +
> +	if (get_user(len, optlen))
> +		return -EFAULT;
> +	if (len < sizeof(value))
> +		return -EINVAL;
> +
> +	lock_sock(sk);
> +	err = -EINVAL;
> +	if (ctx->rx_conf == TLS_SW || ctx->rx_conf == TLS_HW)
> +		value = ctx->rx_no_pad;
> +	release_sock(sk);
> +	if (err)
> +		return err;

Bug: always returns -EINVAL here, because it's assigned a few lines
above unconditionally.

> +
> +	if (put_user(sizeof(value), optlen))
> +		return -EFAULT;
> +	if (copy_to_user(optval, &value, sizeof(value)))
> +		return -EFAULT;
> +
> +	return 0;
> +}
> 

> diff --git a/net/tls/tls_proc.c b/net/tls/tls_proc.c
> index feeceb0e4cb4..0c200000cc45 100644
> --- a/net/tls/tls_proc.c
> +++ b/net/tls/tls_proc.c
> @@ -18,6 +18,7 @@ static const struct snmp_mib tls_mib_list[] = {
>  	SNMP_MIB_ITEM("TlsRxDevice", LINUX_MIB_TLSRXDEVICE),
>  	SNMP_MIB_ITEM("TlsDecryptError", LINUX_MIB_TLSDECRYPTERROR),
>  	SNMP_MIB_ITEM("TlsRxDeviceResync", LINUX_MIB_TLSRXDEVICERESYNC),
> +	SNMP_MIB_ITEM("TlsDecryptRetry", LINUX_MIN_TLSDECRYPTRETRY),
>  	SNMP_MIB_SENTINEL
>  };
>  
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 2bac57684429..7592b6519953 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1601,6 +1601,7 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
>  	if (unlikely(darg->zc && prot->version == TLS_1_3_VERSION &&
>  		     darg->tail != TLS_RECORD_TYPE_DATA)) {
>  		darg->zc = false;
> +		TLS_INC_STATS(sock_net(sk), LINUX_MIN_TLSDECRYPTRETRY);
>  		return decrypt_skb_update(sk, skb, dest, darg);
>  	}

I recall you planned to have two counters:

> You have a point about the more specific counter, let me add a
> counter for NoPad being violated (tail == 0) as well as the overall
> "decryption happened twice" counter.

Did you decide to stick with one?