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

Message-ID: <AM0PR04MB4241CE6318CB6C9C00EEA3FF8B2A0@AM0PR04MB4241.eurprd04.prod.outlook.com>
Date:   Fri, 27 Jul 2018 05:49:44 +0000
From:   Vakul Garg <vakul.garg@....com>
To:     David Miller <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "borisp@...lanox.com" <borisp@...lanox.com>,
        "aviadye@...lanox.com" <aviadye@...lanox.com>,
        "davejwatson@...com" <davejwatson@...com>
Subject: RE: [PATCH net-next] net/tls: Corrected enabling of zero-copy mode



> -----Original Message-----
> From: netdev-owner@...r.kernel.org [mailto:netdev-
> owner@...r.kernel.org] On Behalf Of David Miller
> Sent: Thursday, July 26, 2018 1:59 AM
> To: Vakul Garg <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.

My apologies for sending incorrect patches. I would be more careful next time. 


> 
> 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.