Re: [PATCH 2/4] New string function explicit_bzero (from OpenBSD).

[Zack Weinberg <zackw at panix dot com>]

On 08/19/2016 08:43 AM, Adhemerval Zanella wrote:
> On 18/08/2016 17:53, Torvald Riegel wrote:
>> On Thu, 2016-08-18 at 20:31 +0200, Florian Weimer wrote:
>>> On 08/17/2016 07:19 PM, Zack Weinberg wrote:
>>>> +#ifdef __USE_MISC
>>>> +/* As bzero, but the compiler will not delete a call to this
>>>> +   function, even if S is dead after the call.  Note: this function
>>>> +   has its own implementation file and should not be slurped into
>>>> +   string-inlines.o.  */
>>>> +__extern_inline void
>>>> +explicit_bzero (void *__s, size_t __n)
>>>> +{
>>>> +  memset (__s, '\0', __n);
>>>> +  __glibc_read_memory (__s, __n);
>>>> +}
>>>> +#endif
>>>
>>> __extern_inline can expand to nothing at all, and you would get multiple 
>>> definitions of explicit_bzero this way.

OK, what should I be using instead?

>>> I don't think we want explicit_bzero to be inlined, it's useful to have 
>>> this name in the executable.  Furthermore, we might want to add 
>>> additional state clearing later, so an implementation in libc.so.6 seems 
>>> desirable anyway.

Oddly enough, inlining the memset leads to better security properties
for generated code.  Remember the "key that lives in the vector
registers until it has to be copied onto the stack to be erased" scenario?

---
#include <emmintrin.h>
#include <string.h>

extern void explicit_bzero (void *ptr, size_t size);

extern __m128 get_key ();
extern void encrypt_with_key (__m128 key, void *data, size_t size);

void encrypt (void *data, size_t size)
{
  __m128 key = get_key ();
  encrypt_with_key (key, data, size);
  explicit_bzero (&key, sizeof key);
}
---

(Assume an ABI where __m128 actually gets passed in registers.)

If you expose the memset to inlining,  the key *won't* be copied into
the stack and then immediately erased; the compiler will just dutifully
clear 16 bytes of the stack and then pass them to __glibc_read_memory.
It still won't be erased from the vector registers, but that can only be
fixed with compiler support.

(It also leads to significantly more compact code -- see my experiment
with OpenSSL from last year -- but that's not as important.)

>> I suppose we just want a compiler barrier here though, and don't need a
>> memory barrier in the sense of something that constrains HW reordering.

That is correct; however, the "memory barrier" I was talking about was

    asm volatile ("" ::: "memory");

which is also only a compiler barrier.

> I would also suggest to avoid adding this inline optimization, it just add
> another exported symbol by glibc with little performance benefit. 

The *other* reason why I want to keep the inline optimization is that it
means I don't need to add __explicit_bzero_chk (which, because of
ifuncs, means futzing with *every sysdeps definition of memset*!).  So
either way there's going to be a new exported symbol.

zw