From: Mikael Pettersson <mikpe@it.uu.se>
To: Roel Kluin <roel.kluin@gmail.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>,
Mikael Pettersson <mikpe@it.uu.se>,
"David S. Miller" <davem@davemloft.net>,
linux-crypto@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] sha: prevent removal of memset as dead store in sha1_update()
Date: Thu, 25 Feb 2010 16:56:01 +0100 [thread overview]
Message-ID: <19334.40337.651079.440912@pilspetsen.it.uu.se> (raw)
In-Reply-To: <4B8692E3.9030509@gmail.com>
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@gmail.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.
next prev parent reply other threads:[~2010-02-25 15:56 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-02-25 15:10 [PATCH] sha: prevent removal of memset as dead store in sha1_update() Roel Kluin
2010-02-25 15:10 ` Roel Kluin
2010-02-25 15:17 ` David Miller
2010-02-25 15:31 ` roel kluin
2010-02-25 15:31 ` roel kluin
2010-02-25 15:37 ` David Miller
2010-02-26 11:55 ` Andi Kleen
2010-02-26 14:20 ` Mikael Pettersson
2010-02-26 15:46 ` Mikael Pettersson
2010-02-26 15:55 ` Andi Kleen
2010-02-25 15:56 ` Mikael Pettersson [this message]
2010-02-25 16:16 ` Pekka Enberg
2010-02-25 16:16 ` Pekka Enberg
2010-02-25 16:29 ` Mikael Pettersson
2010-02-25 16:29 ` Mikael Pettersson
2010-02-25 16:33 ` roel kluin
2010-02-25 16:33 ` roel kluin
2010-02-25 17:06 ` roel kluin
2010-02-25 17:06 ` roel kluin
2010-02-25 16:33 ` Brian Gerst
2010-02-25 16:33 ` Brian Gerst
2010-02-25 17:09 ` Mikael Pettersson
2010-02-25 17:09 ` Mikael Pettersson
2010-02-25 17:32 ` Brian Gerst
2010-02-25 17:32 ` Brian Gerst
2010-02-25 19:47 ` Roel Kluin
2010-02-25 20:43 ` Roel Kluin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=19334.40337.651079.440912@pilspetsen.it.uu.se \
--to=mikpe@it.uu.se \
--cc=akpm@linux-foundation.org \
--cc=davem@davemloft.net \
--cc=herbert@gondor.apana.org.au \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=roel.kluin@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.