Re: [PATCH] explicit_bzero final

[Zack Weinberg <zackw@panix.com>]

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.