This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] explicit_bzero final
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