[v2] x86: Harden printf against non-normal long double values (bug 26649)

  The behavior of isnan/__builtin_isnan on bit patterns that do not
correspond to something that the CPU would produce from valid inputs
is currently under-defined in the toolchain. (The GCC built-in and
glibc disagree.)

The isnan check in PRINTF_FP_FETCH in stdio-common/printf_fp.c
assumes the GCC behavior that returns true for non-normal numbers
which are not specified as NaN. (The glibc implementation returns
false for such numbers.)

At present, passing non-normal numbers to __mpn_extract_long_double
causes this function to produce irregularly shaped multi-precision
integers, triggering undefined behavior in __printf_fp_l.

With GCC 10 and glibc 2.32, this behavior is not visible because
__builtin_isnan is used, which avoids calling
__mpn_extract_long_double in this case.  This commit updates the
implementation of __mpn_extract_long_double so that regularly shaped
multi-precision integers are produced in this case, avoiding
undefined behavior in __printf_fp_l.

---
Tested on i686-linux-gnu and x86_64-linux-gnu.

sysdeps/x86/Makefile                    |  4 +++
 sysdeps/x86/ldbl2mpn.c                  |  7 +++++
 sysdeps/x86/tst-ldbl-nonnormal-printf.c | 52 +++++++++++++++++++++++++++++++++
 3 files changed, 63 insertions(+)
  

On Tue, Sep 22, 2020 at 9:37 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> The behavior of isnan/__builtin_isnan on bit patterns that do not
> correspond to something that the CPU would produce from valid inputs
> is currently under-defined in the toolchain. (The GCC built-in and
> glibc disagree.)
>
> The isnan check in PRINTF_FP_FETCH in stdio-common/printf_fp.c
> assumes the GCC behavior that returns true for non-normal numbers
> which are not specified as NaN. (The glibc implementation returns
> false for such numbers.)
>
> At present, passing non-normal numbers to __mpn_extract_long_double
> causes this function to produce irregularly shaped multi-precision
> integers, triggering undefined behavior in __printf_fp_l.
>
> With GCC 10 and glibc 2.32, this behavior is not visible because
> __builtin_isnan is used, which avoids calling
> __mpn_extract_long_double in this case.  This commit updates the
> implementation of __mpn_extract_long_double so that regularly shaped
> multi-precision integers are produced in this case, avoiding
> undefined behavior in __printf_fp_l.
>
> ---
> Tested on i686-linux-gnu and x86_64-linux-gnu.
>
> sysdeps/x86/Makefile                    |  4 +++
>  sysdeps/x86/ldbl2mpn.c                  |  7 +++++
>  sysdeps/x86/tst-ldbl-nonnormal-printf.c | 52 +++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
> index c369faf00d..081cc72e93 100644
> --- a/sysdeps/x86/Makefile
> +++ b/sysdeps/x86/Makefile
> @@ -11,6 +11,10 @@ tests += tst-get-cpu-features tst-get-cpu-features-static \
>  tests-static += tst-get-cpu-features-static
>  endif
>
> +ifeq ($(subdir),math)
> +tests += tst-ldbl-nonnormal-printf
> +endif # $(subdir) == math
> +
>  ifeq ($(subdir),setjmp)
>  gen-as-const-headers += jmp_buf-ssp.sym
>  sysdep_routines += __longjmp_cancel
> diff --git a/sysdeps/x86/ldbl2mpn.c b/sysdeps/x86/ldbl2mpn.c
> index ec8464eef7..867c66ef8d 100644
> --- a/sysdeps/x86/ldbl2mpn.c
> +++ b/sysdeps/x86/ldbl2mpn.c
> @@ -115,6 +115,13 @@ __mpn_extract_long_double (mp_ptr res_ptr, mp_size_t size,
>            && res_ptr[N - 1] == 0)
>      /* Pseudo zero.  */
>      *expt = 0;
> +  else
> +    /* The sign bit is explicit, but add it in case it is missing in
> +       the input.  Otherwise, callers will not be able to produce the
> +       expected multi-precision integer layout by shifting the sign
> +       bit into the MSB.  */
> +    res_ptr[N - 1] |= (mp_limb_t) 1 << (LDBL_MANT_DIG - 1
> +                                       - ((N - 1) * BITS_PER_MP_LIMB));
>
>    return N;
>  }
> diff --git a/sysdeps/x86/tst-ldbl-nonnormal-printf.c b/sysdeps/x86/tst-ldbl-nonnormal-printf.c
> new file mode 100644
> index 0000000000..72797fc33b
> --- /dev/null
> +++ b/sysdeps/x86/tst-ldbl-nonnormal-printf.c
> @@ -0,0 +1,52 @@
> +/* Test printf with x86-specific non-normal long double value.
> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <support/check.h>
> +
> +/* Fill the stack with non-zero values.  This makes a crash in
> +   snprintf more likely.  */
> +static void __attribute__ ((noinline, noclone))
> +fill_stack (void)
> +{
> +  char buffer[65536];
> +  memset (buffer, 0xc0, sizeof (buffer));
> +  asm ("" ::: "memory");
> +}
> +
> +static int
> +do_test (void)
> +{
> +  fill_stack ();
> +
> +  long double value;
> +  memcpy (&value, "\x00\x04\x00\x00\x00\x00\x00\x00\x00\x04", 10);
> +
> +  char buf[30];
> +  int ret = snprintf (buf, sizeof (buf), "%Lg", value);
> +  TEST_COMPARE (ret, strlen (buf));
> +  if (strcmp (buf, "nan") != 0)
> +    /* If snprintf does not recognize the non-normal number as a NaN,
> +       it has added the missing sign bit.  */
> +    TEST_COMPARE_STRING (buf, "3.02201e-4624");
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
>
> --
> Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
> Commercial register: Amtsgericht Muenchen, HRB 153243,
> Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
>

LGTM.

Thanks.
  

* H. J. Lu:

>> +  else
>> +    /* The sign bit is explicit, but add it in case it is missing in
>> +       the input.  Otherwise, callers will not be able to produce the
>> +       expected multi-precision integer layout by shifting the sign
>> +       bit into the MSB.  */
>> +    res_ptr[N - 1] |= (mp_limb_t) 1 << (LDBL_MANT_DIG - 1
>> +                                       - ((N - 1) * BITS_PER_MP_LIMB));

The comment is wrong.  This is not the sign bit.

What about this instead?

    /* Unlike other floating point formats, the most significant bit
       is explicit and expected to be set for normal numbers.  Set it
       in case it is cleared in the input.  Otherwise, callers will
       not be able to produce the expected multi-precision integer
       layout by shifting.  */

And in the test:

    /* If snprintf does not recognize the non-normal number as a NaN,
       it has added the missing explicit MSB.  */

(The commit message should be okay unchanged.)

> LGTM.

Still okay with these changes?

Thanks,
Florian
  

On Tue, Sep 22, 2020 at 10:02 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> >> +  else
> >> +    /* The sign bit is explicit, but add it in case it is missing in
> >> +       the input.  Otherwise, callers will not be able to produce the
> >> +       expected multi-precision integer layout by shifting the sign
> >> +       bit into the MSB.  */
> >> +    res_ptr[N - 1] |= (mp_limb_t) 1 << (LDBL_MANT_DIG - 1
> >> +                                       - ((N - 1) * BITS_PER_MP_LIMB));
>
> The comment is wrong.  This is not the sign bit.
>
> What about this instead?
>
>     /* Unlike other floating point formats, the most significant bit
>        is explicit and expected to be set for normal numbers.  Set it
>        in case it is cleared in the input.  Otherwise, callers will
>        not be able to produce the expected multi-precision integer
>        layout by shifting.  */
>
> And in the test:
>
>     /* If snprintf does not recognize the non-normal number as a NaN,
>        it has added the missing explicit MSB.  */
>
> (The commit message should be okay unchanged.)
>
> > LGTM.
>
> Still okay with these changes?
>

Yes.

Thanks.