linux-kernel - Re: [patch] crypto: sha256-mb - cleanup a || vs

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]

Date:	Fri, 1 Jul 2016 12:13:30 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	Tim Chen <tim.c.chen@...ux.intel.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	"David S. Miller" <davem@...emloft.net>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
	Megha Dey <megha.dey@...el.com>,
	"Wang, Rui Y" <rui.y.wang@...el.com>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	Xiaodong Liu <xiaodong.liu@...el.com>,
	linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Subject: Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

* Herbert Xu <herbert@...dor.apana.org.au> wrote:

> On Fri, Jul 01, 2016 at 09:55:59AM +0200, Ingo Molnar wrote:
> >
> > Plus:
> > 
> > > > >  		/* Compute how many bytes to copy from user buffer into
> > > > >  		 * extra block
> > > > >  		 */
> > 
> > please use the customary (multi-line) comment style:
> 
> This is the customary comment style of the networking stack and
> the crypto API.  So please don't change it.

Guys, do you even read your own code??

That 'standard' is not being enforced consistently at all. Even in this very 
series there's an example of that weird comment not being followed:

+++ b/arch/x86/crypto/sha1-mb/sha1_mb.c
@@ -304,7 +304,7 @@ static struct sha1_hash_ctx *sha1_ctx_mgr_submit(struct sha1_ctx_mgr *mgr,
                /*
                 * Compute how many bytes to copy from user buffer into
                 * extra block

See how this comment block uses the standard coding style, while the next patch 
has this weird coding style:

-       if ((ctx->partial_block_buffer_length) | (len < SHA256_BLOCK_SIZE)) {
+       if ((ctx->partial_block_buffer_length) || (len < SHA256_BLOCK_SIZE)) {
                /* Compute how many bytes to copy from user buffer into
                 * extra block
                 */

The networking code's "exceptionalism" regarding the standard comment style is 
super distracting and in this particular example it resulted in:

 - inconsistent comment styles next to each other,
 - the questionable '|' pattern hiding right next to:
 - pointless parantheses around the (ctx->partial_block_buffer_length),
 - which field name is also a misnomer.

So anyone doing security review of that weird '|' pattern first has to figure out 
whether the 4 ugly code patterns amount to a security problem or not...

One thing that is more harmful that any of the coding styles: the inconsistent 
coding style used by this code.

Btw., as a historic reference, there is nothing sacred about the 'networking 
comments coding style': I was there (way too many years ago) when that comment 
style was introduced by Alan Cox's first TCP/IP code drop, and it was little more 
than just a random inconsistency that people are now treating as gospel...

Thanks,

	Ingo