This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] 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).
--- a/crypt/crypt-entry.c
+++ b/crypt/crypt-entry.c
@@ -142,15 +142,13 @@ __crypt_r (const char *key, const char *salt,
*/
_ufc_output_conversion_r (res[0], res[1], salt, data);
-#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.
libcrypt really does need to refer to __explicit_bzero, not
explicit_bzero. Joseph can explain better than I can, but the
fundamental constraint is that the implementation of a standardized
function ('crypt' is POSIX) is not allowed to refer to nonstandard
user-namespace symbols. This change should have triggered
linknamespace failures.
+/* 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.
+ /* 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.
--- a/include/string.h
+++ b/include/string.h
@@ -100,20 +100,15 @@ extern __typeof (memmem) __memmem;
libc_hidden_proto (__memmem)
libc_hidden_proto (__ffs)
-/* explicit_bzero is used in libcrypt. */
-extern __typeof (explicit_bzero) __explicit_bzero;
-extern __typeof (explicit_bzero) __internal_explicit_bzero;
-libc_hidden_proto (__internal_explicit_bzero)
-extern __typeof (__glibc_read_memory) __internal_glibc_read_memory;
-libc_hidden_proto (__internal_glibc_read_memory)
-/* Honor string.h inlines when present. */
-#if __GNUC_PREREQ (3,4) \
- && ((defined __extern_always_inline \
- && defined __OPTIMIZE__ && !defined __OPTIMIZE_SIZE__ \
- && !defined __NO_INLINE__ && !defined __NO_STRING_INLINES) \
- || (__USE_FORTIFY_LEVEL > 0 && defined __fortify_function))
-# define __explicit_bzero(s,n) explicit_bzero (s,n)
-# define __internal_explicit_bzero(s,n) explicit_bzero (s,n)
+#if IS_IN (libc)
+/* Avoid hidden reference to IFUNC symbol __explicit_bzero_chk. */
+void __explicit_bzero_chk_internal (void *, size_t, size_t)
+ __THROW __nonnull ((1)) attribute_hidden;
+# define explicit_bzero(buf, len) \
+ __explicit_bzero_chk_internal (buf, len, __bos0 (buf))
+#elif !IS_IN (nonlib)
+void __explicit_bzero_chk (void *, size_t, size_t) __THROW __nonnull ((1));
+# define explicit_bzero(buf, len) __explicit_bzero_chk (buf, len, __bos0 (buf))
#endif
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?)
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.