[PATCH] explicit_bzero final

[Florian Weimer]

On 12/14/2016 11:28 PM, Zack Weinberg wrote:

> The remaining question in my mind is whether, in the case where a
> variable's address is only taken in a call to explicit_bzero, we
> should give up on the "hack to prevent the data being copied to
> memory" for the sake of hypothetical future GCC support.  That hack, I
> remind you, is the inline expansion to memset+__glibc_read_memory.  We
> made a huge fuss over that case in the manual, and a couple of people
> were prepared to veto explicit_bzero altogether if we didn't do
> something about it.

Those people were mistaken.  I can't see how you can prevent DSE on 
scalars with current GCC.  I gave that example to show that compiler 
support was needed to implement memset_s (or a full implementation of 
explicit_zero).  It was never intended as something that is particularly 
relevant to real-world code.

> I am very reluctant to give it up, especially as
> I'm still not convinced it's a problem for the compiler (see reply to
> Jeff).

It's a problem because its semantics are not defined in any sensible 
way.  Even explicit_bzero is impossible to specify properly in C 
standardese due to lack of an observable effect on the language level 
(and so is memset_s), but at least it's reasonably obvious what is 
intended.  Instead of your tracking idea, GCC could just clear all stack 
memory and callee-saved registers upon exit from a function which calls 
explicit_bzero.  It's still an approximation, but any implementation 
retrofitted on an existing compiler will be.
	
> -#ifdef _LIBC
>    /*
>     * Erase key-dependent intermediate data.  Data dependent only on
>     * the salt is not considered sensitive.
>     */
> -  __explicit_bzero (ktab, sizeof (ktab));
> -  __explicit_bzero (data->keysched, sizeof (data->keysched));
> -  __explicit_bzero (res, sizeof (res));
> -#endif
> +  explicit_bzero (ktab, sizeof (ktab));
> +  explicit_bzero (data->keysched, sizeof (data->keysched));
> +  explicit_bzero (res, sizeof (res));
>
> The _LIBC ifdeffage is vestigial, but should probably be left alone in
> a patch that isn't about that.

Huh?  I think it's a new addition.

> +/* This is the generic definition of __explicit_bzero_chk.  The
> +   __explicit_bzero_chk symbol is used as the implementation of
> +   explicit_bzero throughout glibc.  If this file is overriden by an
> +   architecture, both __explicit_bzero_chk and
> +   __explicit_bzero_chk_internal have to be defined (the latter not as
> +   an IFUNC).  */
>
> This file is not in sysdeps/generic, so it cannot be overridden (or is
> that no longer the case? If so, why do we still have sysdeps/generic?)
> and I don't think we need the capability to override it.  Better we
> should get libc-internal references to memset going to the proper ifunc
> for the architecture.

It can be overridden, and it has to be, otherwise you end up with 
multiple definitions of the same symbol when linking libc.so.

Two symbols are needed because on some architectures, hidden references 
to IFUNC symbols are not supported because calls to hidden functions are 
always direct and do not use the GOT.

> +  /* Compiler barrier.  */
> +  asm volatile ("" ::: "memory");
> +}
>
> I do not understand why you have reverted to an older, inferior
> compiler barrier.  This was extensively hashed out quite some time ago.

I did that because asm volatile ("") is essentially a NOP in this 
context.  It certainly does not prevent DSE of the preceding memset.  In 
contrast, the memory clobber ensures that if the thing-to-be-zeroed is 
in memory.

> Oh, I see why you're not getting linknamespace failures from the
> libcrypt change: you've implicitly fortified all those calls, which
> has the side effect of making them use an impl-namespace symbol.  It
> makes sense as a testing strategy, but it doesn't feel like the right
> move for the committed patch (better to leave that to an all-or-nothing
> "fortify libc internally" switch, ne?)

I have no firm opinion on that.  It avoids adding another GLIBC_PRIVATE 
symbol, and I think the current set of symbols is also compatible with 
future IFUNC-based optimizations.

> What we _could_ do is
>
> #if IS_IN (libc)
> # define explicit_bzero(s, n) __internal_explicit_bzero (s, n)
> #else
> # define explicit_bzero(s, n) __explicit_bzero (s, n)
> #endif
>
> which would allow libcrypt's source code to use the unmangled names.
> I think that's something we're trying to do for other string
> functions, so perhaps it makes sense.

Other string functions use asm aliases, which also apply to GCC built-ins.

Thanks,
Florian