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 <[email protected]>
---
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);
From: Roel Kluin <[email protected]>
Date: Thu, 25 Feb 2010 16:10:27 +0100
> 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 <[email protected]>
Solution is wrong and overkill in my mind.
It's overkill because the whole reason it's using a stack buffer is to
avoid the overhead of a kmalloc() call.
And it's wrong because the reason the memset() is there seems to be
to clear out key information that might exist kernel stack so that
it's more difficult for rogue code to get at things.
On Thu, Feb 25, 2010 at 4:17 PM, David Miller <[email protected]> wrote:
> From: Roel Kluin <[email protected]>
> Date: Thu, 25 Feb 2010 16:10:27 +0100
>
>> 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 <[email protected]>
>
> Solution is wrong and overkill in my mind.
>
> It's overkill because the whole reason it's using a stack buffer is to
> avoid the overhead of a kmalloc() call.
>
> And it's wrong because the reason the memset() is there seems to be
> to clear out key information that might exist kernel stack so that
> it's more difficult for rogue code to get at things.
If the memset is optimized away then the clear out does not occur. Do you
know a different way to fix this? I observed this with:
$ gcc -O2 test.c;./a.out
and It shows (on my box) "...S.e.c.r.e.t..."
$ cat test.c
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#define ON_STACK 1
void foo()
{
char password[] = "secret";
password[0]='S';
printf ("Don't show again: %s\n", password);
memset(password, 0, sizeof(password));
}
void foo2()
{
char* password = malloc(7);
strncpy (password, "secret" , 7);
password[6] = '\0';
password[0] = 'S';
printf ("Don't show again: %s\n", password);
//memset(password, 0, 7);
free(password);
}
int main(int argc, char* argv[])
{
#if ON_STACK == 1
foo();
#else
foo2();
#endif
int i;
char foo3[] = "hoi";
printf ("foo1:%s\n", foo3);
char* bar = &foo3[0];
for (i = -50; i < 50; i++)
printf ("%c.", bar[i]);
printf("\n");
return 0;
}
From: roel kluin <[email protected]>
Date: Thu, 25 Feb 2010 16:31:36 +0100
> On Thu, Feb 25, 2010 at 4:17 PM, David Miller <[email protected]> wrote:
>> From: Roel Kluin <[email protected]>
>> Date: Thu, 25 Feb 2010 16:10:27 +0100
>>
>>> 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 <[email protected]>
>>
>> Solution is wrong and overkill in my mind.
>>
>> It's overkill because the whole reason it's using a stack buffer is to
>> avoid the overhead of a kmalloc() call.
>>
>> And it's wrong because the reason the memset() is there seems to be
>> to clear out key information that might exist kernel stack so that
>> it's more difficult for rogue code to get at things.
>
> If the memset is optimized away then the clear out does not occur. Do you
> know a different way to fix this?
Not offhand. Maybe we can make some external helper function for the
crypto layer that just does the memset, but is not visible from any of
the call sites. GCC doesn't know the side effects, so it can't
elide the call to that helper function.
That could be subverted by whole-program-optimizations but
currently that really isn't something to worry about.
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 <[email protected]>
> ---
> 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.
On Thu, Feb 25, 2010 at 5:56 PM, Mikael Pettersson <[email protected]> wrote:
> 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.
So is there some "do not optimize" GCC magic that we could use for a
memzero_secret() helper function?
Pekka
Pekka Enberg writes:
> On Thu, Feb 25, 2010 at 5:56 PM, Mikael Pettersson <[email protected]> wrote:
> > 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.
>
> So is there some "do not optimize" GCC magic that we could use for a
> memzero_secret() helper function?
I guess there's some -fno-builtin-... that might achieve this effect,
but that would disable all memset optimizations, not just those affecting
sensitive data.
You'd want a function attribute or magic type annotation and apply it
only to the specific cases where it's needed. Alas, I know of no such
attribute or annotation. ('volatile' doesn't work, I tried that.)
Ask on [email protected].
On Thu, Feb 25, 2010 at 5:16 PM, Pekka Enberg <[email protected]> wrote:
> On Thu, Feb 25, 2010 at 5:56 PM, Mikael Pettersson <[email protected]> wrote:
>> 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.
>
> So is there some "do not optimize" GCC magic that we could use for a
> memzero_secret() helper function?
>
> Pekka
>
*(volatile char *)p = *(volatile char *)p;
appears to work when called after the memset:
---
inline void ensure_memset(void* p)
{
*(volatile char *)p = *(volatile char *)p;
}
void foo()
{
char password[] = "secret";
password[0]='S';
printf ("Don't show again: %s\n", password);
memset(password, 0, sizeof(password));
ensure_memset(password);
}
int main(int argc, char* argv[])
{
foo();
int i;
char foo3[] = "";
char* bar = &foo3[0];
for (i = -50; i < 50; i++)
printf ("%c.", bar[i]);
printf("\n");
return 0;
}
On Thu, Feb 25, 2010 at 10:56 AM, Mikael Pettersson <[email protected]> wrote:
> 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 <[email protected]>
> > ---
> > 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.
Would barrier() (which is a simple memory clobber) after the memset work?
--
Brian Gerst
On Thu, Feb 25, 2010 at 5:33 PM, roel kluin <[email protected]> wrote:
> On Thu, Feb 25, 2010 at 5:16 PM, Pekka Enberg <[email protected]> wrote:
>> On Thu, Feb 25, 2010 at 5:56 PM, Mikael Pettersson <[email protected]> wrote:
>>> 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.
>>
>> So is there some "do not optimize" GCC magic that we could use for a
>> memzero_secret() helper function?
>>
>> Pekka
>>
>
> *(volatile char *)p = *(volatile char *)p;
>
> appears to work when called after the memset:
Or similar to suggested here:
https://www.securecoding.cert.org/confluence/display/cplusplus/MSC06-CPP.+Be+aware+of+compiler+optimization+when+dealing+with+sensitive+data
This memzero_secret() appears to work:
void *memzero_secret(void *v, size_t n) {
volatile unsigned char *p = v;
while (n--)
*p++ = 0;
return v;
}
---
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
void *memzero_secret(void *v, size_t n) {
volatile unsigned char *p = v;
while (n--)
*p++ = 0;
return v;
}
void foo()
{
char password[] = "secret";
password[0]='S';
printf ("Don't show again: %s\n", password);
memzero_secret(password, sizeof(password));
//memset(password, 0, sizeof(password));
}
int main(int argc, char* argv[])
{
foo();
int i;
char foo3[] = "";
char* bar = &foo3[0];
for (i = -50; i < 50; i++)
printf ("%c.", bar[i]);
printf("\n");
return 0;
}
Brian Gerst wrote:
> Would barrier() (which is a simple memory clobber) after the memset work?
I don't know. It's implemented as an asm with a "memory" clobber,
but I wouldn't bet on that forcing previous writes to a dying object
to actally be performed (it would have to have a data-dependency on
the dying object, but I don't think there is one).
void secure_bzero(void *p, size_t n)
{
memset(p, 0, n);
asm("" : : "m"(*(char*)p));
}
seems to work, but as the object in general will be larger than a
single byte, I'd like to see some confirmation from the gcc folks
first that this will in fact work.
On Thu, Feb 25, 2010 at 12:09 PM, Mikael Pettersson <[email protected]> wrote:
> Brian Gerst wrote:
>> Would barrier() (which is a simple memory clobber) after the memset work?
>
> I don't know. It's implemented as an asm with a "memory" clobber,
> but I wouldn't bet on that forcing previous writes to a dying object
> to actally be performed (it would have to have a data-dependency on
> the dying object, but I don't think there is one).
>From the GCC manual, section 5.37:
If your assembler instructions access memory in an unpredictable
fashion, add `memory' to the list of clobbered registers. This will
cause GCC to not keep memory values cached in registers across the
assembler instruction and not optimize stores or loads to that memory.
You will also want to add the volatile keyword if the memory affected
is not listed in the inputs or outputs of the asm, as the `memory'
clobber does not count as a side-effect of the asm.
--
Brian Gerst
Op 25-02-10 18:32, Brian Gerst schreef:
> On Thu, Feb 25, 2010 at 12:09 PM, Mikael Pettersson <[email protected]> wrote:
>> Brian Gerst wrote:
>>> Would barrier() (which is a simple memory clobber) after the memset work?
>>
>> I don't know. It's implemented as an asm with a "memory" clobber,
>> but I wouldn't bet on that forcing previous writes to a dying object
>> to actally be performed (it would have to have a data-dependency on
>> the dying object, but I don't think there is one).
>
>>From the GCC manual, section 5.37:
> If your assembler instructions access memory in an unpredictable
> fashion, add `memory' to the list of clobbered registers. This will
> cause GCC to not keep memory values cached in registers across the
> assembler instruction and not optimize stores or loads to that memory.
> You will also want to add the volatile keyword if the memory affected
> is not listed in the inputs or outputs of the asm, as the `memory'
> clobber does not count as a side-effect of the asm.
>
> --
> Brian Gerst
>
Also from that document:
If you know how large the accessed memory is, you can add it as input or
output but if this is not known, you should add memory. As an example, if
you access ten bytes of a string, you can use a memory input like:
{"m"( ({ struct { char x[10]; } *p = (void *)ptr ; *p; }) )}.
I am new to assembly but does this mean we could use something like:
#define SECURE_BZERO(x) do { \
memset(x, 0, sizeof(x)); \
asm("" : :"m"( ({ struct { char __y[ARRAY_SIZE(x)]; } *__z = \
(void *)x ; *__z; }) )); \
} while(0)
Roel
> Also from that document:
>
> If you know how large the accessed memory is, you can add it as input or
> output but if this is not known, you should add memory. As an example, if
> you access ten bytes of a string, you can use a memory input like:
>
> {"m"( ({ struct { char x[10]; } *p = (void *)ptr ; *p; }) )}.
>
> does this mean we could use something like:
>
> #define SECURE_BZERO(x) do { \
> memset(x, 0, sizeof(x)); \
> asm("" : :"m"( ({ struct { char __y[ARRAY_SIZE(x)]; } *__z = \
> (void *)x ; *__z; }) )); \
> } while(0)
or rather for not just char arrays:
#define SECURE_BZERO(x) do { \
memset(x, 0, sizeof(x)); \
asm("" :: "m" ( ({ \
struct { \
typeof(x[0]) __y[ARRAY_SIZE(x)];\
} *__z = (void *)x; \
*__z; \
}) )); \
} while(0)
This appears to work in my testcase:
---
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#define SECURE 1
#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
#define SECURE_BZERO(x) do { \
memset(x, 0, sizeof(x)); \
asm("" :: "m" ( ({ \
struct { \
typeof(x[0]) __y[ARRAY_SIZE(x)];\
} *__z = (void *)x; \
*__z; \
}) )); \
} while(0)
void foo()
{
char password[] = "secret";
password[0]='S';
printf ("Don't show again: %s\n", password);
#if SECURE == 1
SECURE_BZERO(password);
#else
memset(password, 0, sizeof(password));
#endif
}
void foo1()
{
int nrs[] = {1,1,2,3,4,5,6,7};
nrs[0] = 0;
int i = 8;
printf ("Don't show again:\n");
while (i--)
printf ("%u\n", nrs[i]);
#if SECURE == 1
SECURE_BZERO(nrs);
#else
memset(nrs, 0, sizeof(nrs));
#endif
}
int main(int argc, char* argv[])
{
foo();
int i;
char foo3[] = "";
char* bar = &foo3[0];
for (i = -50; i < 50; i++)
printf ("%c.", bar[i]);
printf("\n\n");
foo1();
int foo4 = 20;
int* ber = &foo4;
for (i = -50; i < 50; i++)
printf ("%u_", ber[i]);
printf("\n");
return 0;
}
roel kluin <[email protected]> writes:
>> And it's wrong because the reason the memset() is there seems to be
>> to clear out key information that might exist kernel stack so that
>> it's more difficult for rogue code to get at things.
>
> If the memset is optimized away then the clear out does not occur. Do you
> know a different way to fix this? I observed this with:
You could always cast to volatile before memsetting?
-Andi
--
[email protected] -- Speaking for myself only.
Andi Kleen writes:
> roel kluin <[email protected]> writes:
>
> >> And it's wrong because the reason the memset() is there seems to be
> >> to clear out key information that might exist kernel stack so that
> >> it's more difficult for rogue code to get at things.
> >
> > If the memset is optimized away then the clear out does not occur. Do you
> > know a different way to fix this? I observed this with:
>
> You could always cast to volatile before memsetting?
I tried that and it doesn't work. Furthermore passing a volatile void *
to a function expecting a void * provokes a compiler warning.
I currently think that defining and using
void secure_bzero(void *p, size_t n)
{
memset(p, 0, n);
/* We need for this memset() to be performed even if *p
* is about to disappear (a local auto variable going out
* of scope or some dynamic memory being kfreed()).
* Thus we need to fake a "use" of *p here.
* barrier() achieves that effect, and much more.
* TODO: find a better alternative to barrier() here.
*/
barrier();
}
would be a first good step. We can then ask the gcc folks for
a weaker alternative to barrier() that's guaranteed to keep the
object at [p, p+n[ live.
Mikael Pettersson writes:
> Andi Kleen writes:
> > roel kluin <[email protected]> writes:
> >
> > >> And it's wrong because the reason the memset() is there seems to be
> > >> to clear out key information that might exist kernel stack so that
> > >> it's more difficult for rogue code to get at things.
> > >
> > > If the memset is optimized away then the clear out does not occur. Do you
> > > know a different way to fix this? I observed this with:
> >
> > You could always cast to volatile before memsetting?
>
> I tried that and it doesn't work. Furthermore passing a volatile void *
> to a function expecting a void * provokes a compiler warning.
>
> I currently think that defining and using
>
> void secure_bzero(void *p, size_t n)
> {
> memset(p, 0, n);
> /* We need for this memset() to be performed even if *p
> * is about to disappear (a local auto variable going out
> * of scope or some dynamic memory being kfreed()).
> * Thus we need to fake a "use" of *p here.
> * barrier() achieves that effect, and much more.
> * TODO: find a better alternative to barrier() here.
> */
> barrier();
Instead of barrier(), this works with gcc-3.2.3 up to gcc-4.4.3
for the purpose of making the memset() not disappear:
{
struct s { char c[n]; };
asm("" : : "m"(*(struct s *)p));
}
Every byte in the [p,p+n[ range must be used. If you only use the
first byte, via e.g. asm("" :: "m"(*(char*)p)), then the compiler
_will_ skip scrubbing bytes beyond the first.
An explicit loop that uses each byte individually also works, but
results in awful code with older compilers.
> }
> Instead of barrier(), this works with gcc-3.2.3 up to gcc-4.4.3
> for the purpose of making the memset() not disappear:
>
> {
> struct s { char c[n]; };
> asm("" : : "m"(*(struct s *)p));
> }
>
> Every byte in the [p,p+n[ range must be used. If you only use the
> first byte, via e.g. asm("" :: "m"(*(char*)p)), then the compiler
> _will_ skip scrubbing bytes beyond the first.
>
> An explicit loop that uses each byte individually also works, but
> results in awful code with older compilers.
I suspect it would be worth asking for a __builtin for this.
iirc Visual C already has one.
BTW I remember finding a few more such cases in random user space
code in past code review.
-Andi
--
[email protected] -- Speaking for myself only.