netdev - Re: [PATCH net-next] net/tls: Corrected enabling of zero-copy mode

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: <20180725.132836.2058461941832003627.davem@davemloft.net>
Date:   Wed, 25 Jul 2018 13:28:36 -0700 (PDT)
From:   David Miller <davem@...emloft.net>
To:     vakul.garg@....com
Cc:     netdev@...r.kernel.org, borisp@...lanox.com, aviadye@...lanox.com,
        davejwatson@...com
Subject: Re: [PATCH net-next] net/tls: Corrected enabling of zero-copy mode

From: Vakul Garg <vakul.garg@....com>
Date: Mon, 23 Jul 2018 21:00:06 +0530

> @@ -787,7 +787,7 @@ int tls_sw_recvmsg(struct sock *sk,
>  	target = sock_rcvlowat(sk, flags & MSG_WAITALL, len);
>  	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
>  	do {
> -		bool zc = false;
> +		bool zc;
>  		int chunk = 0;
>  
>  		skb = tls_wait_data(sk, flags, timeo, &err);
 ...
> @@ -836,6 +835,7 @@ int tls_sw_recvmsg(struct sock *sk,
>  				if (err < 0)
>  					goto fallback_to_reg_recv;
>  
> +				zc = true;
>  				err = decrypt_skb_update(sk, skb, sgin, &zc);
>  				for (; pages > 0; pages--)
>  					put_page(sg_page(&sgin[pages]));
> @@ -845,6 +845,7 @@ int tls_sw_recvmsg(struct sock *sk,
>  				}
>  			} else {
>  fallback_to_reg_recv:
> +				zc = false;
>  				err = decrypt_skb_update(sk, skb, NULL, &zc);
>  				if (err < 0) {
>  					tls_err_abort(sk, EBADMSG);
> -- 
> 2.13.6
> 

This will leave a code path where 'zc' is evaluated but not initialized to
any value.

And that's the path taken when ctx->decrypted is true.  The code after
your changes looks like:

		bool zc;
 ...
		if (!ctx->decrypted) {

 ... assignments to 'zc' happen in this code block

			ctx->decrypted = true;
		}

		if (!zc) {

So when ctx->decrypted it true, the if(!zc) condition runs on an
uninitialized value.

I have to say that your TLS changes are becomming quite a time sink
for two reasons.

First, you are making a lot of changes that seem not so needed, and
whose value is purely determined by taste.  I'd put the
msg_data_left() multiple evaluation patch into this category.

The rest require deep review and understanding of the complicated
details of the TLS code, and many of them turn out to be incorrect.

As I find more errors in your submissions, I begin to scrutinize your
patches even more.  Thus, review of your changes takes even more time.

And it isn't helping that there are not a lot of other developers
helping actively to review your changes.

I would like to just make a small request to you, that you concentrate
on fixing clear bugs and clear issues that need to be resolved.

Thank you.