Re: [PATCH] explicit_bzero final

[Zack Weinberg <zackw at panix dot com>]

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