linux-kernel - Re: [PATCH] sha: prevent removal of memset as dead store in sha1

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:	Thu, 25 Feb 2010 16:56:01 +0100
From:	Mikael Pettersson <mikpe@...uu.se>
To:	Roel Kluin <roel.kluin@...il.com>
Cc:	Herbert Xu <herbert@...dor.apana.org.au>,
	Mikael Pettersson <mikpe@...uu.se>,
	"David S. Miller" <davem@...emloft.net>,
	linux-crypto@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sha: prevent removal of memset as dead store in sha1_update()

Roel Kluin writes:
 > Due to optimization A call to memset() may be removed as a dead store when
 > the buffer is not used after its value is overwritten.
 > 
 > Signed-off-by: Roel Kluin <roel.kluin@...il.com>
 > ---
 > see http://cwe.mitre.org/data/slices/2000.html#14
 > 
 > checkpatch.pl, compile and sparse tested. Comments?
 > 
 > diff --git a/crypto/sha1_generic.c b/crypto/sha1_generic.c
 > index 0416091..86de0da 100644
 > --- a/crypto/sha1_generic.c
 > +++ b/crypto/sha1_generic.c
 > @@ -49,8 +49,8 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 >  	src = data;
 >  
 >  	if ((partial + len) > 63) {
 > -		u32 temp[SHA_WORKSPACE_WORDS];
 > -
 > +		u32 *temp = kzalloc(SHA_WORKSPACE_WORDS * sizeof(u32),
 > +				GFP_KERNEL);
 >  		if (partial) {
 >  			done = -partial;
 >  			memcpy(sctx->buffer + partial, data, done + 64);
 > @@ -64,6 +64,7 @@ static int sha1_update(struct shash_desc *desc, const u8 *data,
 >  		} while (done + 63 < len);
 >  
 >  		memset(temp, 0, sizeof(temp));
 > +		kfree(temp);
 >  		partial = 0;
 >  	}
 >  	memcpy(sctx->buffer + partial, src, len - done);

At best this might solve the issue right now, but it's not
future-proof by any margin.

One problem is that just like the lifetimes of auto variables are
known to the compiler, allowing dead store elimination (DSE) on them,
there is development going on to make malloc() and free() known to
the compiler. I don't think it's complete yet, but once free() is
known, the sequence "memset(p, 0, n); free(p);" will obviously be
DSE:d just like in the current case with the auto variable.

And as soon as gcc can optimize malloc() and free(), you can be sure that
some eager kernel hacker will mark the kernel's allocators accordingly,
and then we're back to square one.

I fear that the only portable (across compiler versions) and safe
solution is to invoke an assembly-coded dummy function with prototype

	void use(void *p);

and rewrite the code above as

	{
		u32 temp[...];
		...
		memset(temp, 0, sizeof temp);
		use(temp);
	}

This forces the compiler to consider the buffer live after the
memset, so the memset cannot be eliminated.

The reason the use() function needs to be in assembly code is that
with link-time optimizations soon commonplace (LTO in gcc-4.5),
a compiler can possibly discover that even an out-of-line function

	void use(void *p) { }

doesn't in fact use *p, which then enables (in theory) the
preceeding memset() to be DSE:d.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/