memset() in crypto code?

[Sandy Harris]

There was recently a patch to the random driver to replace memset()
because, according to the submitter, gcc sometimes optimises memset()
away which might leave data unnecessarily exposed. The solution
suggested was a function called memzero_explicit(). There was a fair
bit of discussion and the patch was accepted.

In the crypto directory of the kernel source I have:

$ grep memset *.c | wc -l
133
$

I strongly suspect some of these should be fixed.

[Daniel Borkmann]

Hi Sandy,

Post by Sandy Harris
There was recently a patch to the random driver to replace memset()
because, according to the submitter, gcc sometimes optimises memset()
away which might leave data unnecessarily exposed. The solution
suggested was a function called memzero_explicit(). There was a fair
bit of discussion and the patch was accepted.
$ grep memset *.c | wc -l
133
$
I strongly suspect some of these should be fixed.

I have submitted it here one month ago for crypto and it's still
awaiting to be applied:

http://www.spinics.net/lists/linux-crypto/msg11965.html

As the random driver patch has been applied to random -dev, it will be
available from 3.18 onwards, but the dependency for crypto is currently
there, that's why I asked Ted to take it through his tree; hopefully
this will happen soonish (but I haven't heard anything back ever since) ...

Thanks!

Daniel

[Jason Cooper]

Post by Sandy Harris
There was recently a patch to the random driver to replace memset()
because, according to the submitter, gcc sometimes optimises memset()
away which might leave data unnecessarily exposed. The solution
suggested was a function called memzero_explicit(). There was a fair
bit of discussion and the patch was accepted.

Do you have a pointer?

Post by Sandy Harris
$ grep memset *.c | wc -l
133
$
I strongly suspect some of these should be fixed.

It seems this is a common topic:

http://www.daemonology.net/blog/2014-09-04-how-to-zero-a-buffer.html

and part 2:

http://www.daemonology.net/blog/2014-09-06-zeroing-buffers-is-insufficient.html

Summary: It's not just the memory we should be concerned about, there's also
the stack and the registers.

memzero_explicit() is a good start, but I think the articles raise two
important points. a) register data temporarily stored on the stack via
compiler optimizations, and b) aesni register contents left behind after
use.

I suspect (a) is an unquantified problem on embedded systems, and (b)
may extend to embedded hardware crypto implementations.

thx,

Jason.

[Sandy Harris]

Post by Jason Cooper

Post by Sandy Harris
There was recently a patch to the random driver to replace memset()
because, according to the submitter, gcc sometimes optimises memset()
away which might leave data unnecessarily exposed. The solution
suggested was a function called memzero_explicit(). There was a fair
bit of discussion and the patch was accepted.

Do you have a pointer?

https://lkml.org/lkml/2014/8/25/497

[Jason Cooper]

Post by Sandy Harris

Post by Jason Cooper

Post by Sandy Harris
There was recently a patch to the random driver to replace memset()
because, according to the submitter, gcc sometimes optimises memset()
away which might leave data unnecessarily exposed. The solution
suggested was a function called memzero_explicit(). There was a fair
bit of discussion and the patch was accepted.

Do you have a pointer?

https://lkml.org/lkml/2014/8/25/497

Ok, that was the same thread I found. I was looking for the 'fair bit
of discussion' part. ;-)

thx,

Jason.

[Sandy Harris]

Post by Sandy Harris
There was recently a patch to the random driver to replace memset()
because, according to the submitter, gcc sometimes optimises memset()
away ...

memzero_explicit() is a good start, ...

As I see it, memzero_explicit() is a rather ugly kluge, albeit an
acceptable one in the circumstances.

A real fix would make memset() do the right thing reliably; if the
programmer puts in memset( x, 0, nbytes) then the memory should be
cleared, no ifs or buts. I do not know or care if that means changes
in the compiler or in the library code or even both, but the fix
should make the standard library code work right, not require adding a
new function and expecting everyone to use it.

[Daniel Borkmann]

...

Post by Sandy Harris

Post by Sandy Harris
There was recently a patch to the random driver to replace memset()
because, according to the submitter, gcc sometimes optimises memset()
away ...

memzero_explicit() is a good start, ...

As I see it, memzero_explicit() is a rather ugly kluge, albeit an
acceptable one in the circumstances.

Right.

Post by Sandy Harris
A real fix would make memset() do the right thing reliably; if the
programmer puts in memset( x, 0, nbytes) then the memory should be
cleared, no ifs or buts. I do not know or care if that means changes
in the compiler or in the library code or even both, but the fix
should make the standard library code work right, not require adding a
new function and expecting everyone to use it.

That would be a desirable goal, ideally perhaps as a built-in from
the compiler itself, just as memset(). Applications such as openssh
implement for the very same purpose their bzero_explicit() variant
just as well.

[Sandy Harris]

I have started a thread about this on the gcc help mailing list
https://gcc.gnu.org/ml/gcc-help/2014-10/msg00047.html

We might consider replacinging memzero_explicit with memset_s() since
that is in the C!! standard, albeit I think as optional. IBM, Apple,
NetBSD, ... have that.
https://mail-index.netbsd.org/tech-userlevel/2012/02/24/msg006125.html

[Daniel Borkmann]

Post by Sandy Harris
I have started a thread about this on the gcc help mailing list
https://gcc.gnu.org/ml/gcc-help/2014-10/msg00047.html

Great, perhaps you want to pass a patch proposal to gcc folks?

Post by Sandy Harris
We might consider replacinging memzero_explicit with memset_s() since
that is in the C!! standard, albeit I think as optional. IBM, Apple,
NetBSD, ... have that.
https://mail-index.netbsd.org/tech-userlevel/2012/02/24/msg006125.html

The patch you point to for NetBSD does nothing else what memzero_explicit()
or bzero_explicit() variants already internally do, only that they're
discussing about whether to adopt it or not in their user space libc ...

Cheers,
Daniel