memset() in crypto code?

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.

Re: memset() in crypto code?

[Daniel Borkmann]

Hi Sandy,

On 10/05/2014 05:09 AM, Sandy Harris wrote:
> 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.

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

Re: memset() in crypto code?

[Jason Cooper]

On Sat, Oct 04, 2014 at 11:09:40PM -0400, Sandy Harris wrote:
> 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?

> 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.

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.

Re: memset() in crypto code?

[Sandy Harris]

On Mon, Oct 6, 2014 at 1:44 PM, Jason Cooper <jason@lakedaemon.net> wrote:

> On Sat, Oct 04, 2014 at 11:09:40PM -0400, Sandy Harris wrote:
>> 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

Re: memset() in crypto code?

[Jason Cooper]

On Mon, Oct 06, 2014 at 01:59:06PM -0400, Sandy Harris wrote:
> On Mon, Oct 6, 2014 at 1:44 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> 
> > On Sat, Oct 04, 2014 at 11:09:40PM -0400, Sandy Harris wrote:
> >> 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.

Re: memset() in crypto code?

[Sandy Harris]

On Mon, Oct 6, 2014 at 1:44 PM, Jason Cooper <jason@lakedaemon.net> wrote:

> On Sat, Oct 04, 2014 at 11:09:40PM -0400, Sandy Harris wrote:

>> 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.

Re: memset() in crypto code?

[Daniel Borkmann]

On 10/06/2014 08:52 PM, Sandy Harris wrote:
> On Mon, Oct 6, 2014 at 1:44 PM, Jason Cooper <jason@lakedaemon.net> wrote:
>> On Sat, Oct 04, 2014 at 11:09:40PM -0400, Sandy Harris wrote:
...
>>> 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.

> 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.

Re: memset() in crypto code?

[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

Re: memset() in crypto code?

[Daniel Borkmann]

On 10/08/2014 04:30 AM, Sandy Harris wrote:
> 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?

> 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