explicit_bzero final

On Wed, Dec 14, 2016 at 12:05 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 12/14/2016 02:15 PM, Florian Weimer wrote:
>> On 12/14/2016 02:04 AM, Zack Weinberg wrote:
>>>
>>> We also have a nasty interaction between internal PLT bypass and
>>> ifuncs which means that a hypothetical __explicit_bzero_chk is
>>> ridiculously difficult to implement.  I tried that once already and it
>>> did not go well.
>>
>> I'm looking into this aspect right now.
>
> This patch on top of yours implements proper explict_bzero and
> __explicit_bzero_chk symbols.  I put it through build-many-glibcs, and it
> results in the expected ABI everywhere.

I appreciate your having tried this; the patch has a number of
correctable problems (see below) but it does demonstrate that
__explicit_bzero_chk is not a lost cause.

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

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?)

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.

On Wed, 14 Dec 2016, Zack Weinberg wrote:

> 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?)

Files outside sysdeps/generic can be overridden.

Internal headers used from more than one directory and also overridden by 
some configurations belong in sysdeps/generic: include/ comes before 
sysdeps so isn't suitable for headers that get overridden, while if the 
header is in some other directory it's not visible for compilations within 
another directory.  There may be other cases that need to be in 
sysdeps/generic as well.

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

On 12/15/2016 10:08 AM, Florian Weimer wrote:
> 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.

I meant “caller-saved registers”, of course.

Florian

On 12/15/2016 04:08 AM, Florian Weimer wrote:
> 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.

Hmm, OK.  I'm going to send a new patch which merges yours and mine and
adjusts the manual accordingly.  I really want to get this landed
tomorrow, before I go out of town -- otherwise it's going to miss 2.25,
and this has been dragging on _quite_ long enough.

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

Oh, huh, you're right.  I don't remember why I did that.  Certainly
nobody's gonna be merging this back with UFC, so it can go.

>> +/* 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.

??? Your patch doesn't have any overrides of it.

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

Right, I remember that this was a problem before.

>> +  /* 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.

I meant as opposed to the out-of-line __glibc_read_memory.  I suppose an
all-memory clobber is not going to generate worse code than that as long
as explicit_bzero.c remains separately compiled, and it's less likely to
break (but _will_ generate worse code) if LTO-into-glibc happens before
compiler support for explicit_bzero, so I can live with it.

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

I think I'm going to leave it your way, both because it'll get the patch
done faster, and because the crypt code is awfully dusty and not at all
performance-critical.

zw

On 12/16/2016 12:31 AM, Zack Weinberg wrote:
>>> +/* 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.
> ??? Your patch doesn't have any overrides of it.

I meant it has to be overridden if you want to introduce 
architecture-specific implementations of explicit_bzero.  The patch 
doesn't contain any, but should be compatible with future development.

Due the transitive nature of IFUNCs and the desire to avoid indirect 
calls, all memset/__memset_chk variants need an 
explicit_bzero/__explicit_bzero_chk wrapper, so this is quite involved.

Thanks,
Florian