Message ID | 20190202142723.7730-1-hjl.tools@gmail.com |
---|---|
State | New |
Headers | show |
Series | x86-64 memcmp: Use unsigned Jcc instructions on size | expand |
* H. J. Lu: > Since the size argument is unsigned. we should use unsigned Jcc > instructions, instead of signed to check size. > > Tested on x86-64 and x32, with and without --disable-multi-arch. Does this impact x86-64 at all (technically), consider that an object size larger than SSIZE_MAX would be undefined anyway? It seems that on x32, it can give incorrect results if the sign bit on the 64-bit register is set. In this sense, it is similar to CVE-2019-6488 in impact, right? If we decide to treat this as a security vulnerability, we need a new CVE ID because the version range is different (bug 24155 was not fixed in the 2.29 release). Thanks, Florian
On Sat, Feb 2, 2019 at 6:57 AM Florian Weimer <fweimer@redhat.com> wrote: > > * H. J. Lu: > > > Since the size argument is unsigned. we should use unsigned Jcc > > instructions, instead of signed to check size. > > > > Tested on x86-64 and x32, with and without --disable-multi-arch. > > Does this impact x86-64 at all (technically), consider that an object > size larger than SSIZE_MAX would be undefined anyway? I don't think we will hit it on x86-64. > It seems that on x32, it can give incorrect results if the sign bit on > the 64-bit register is set. In this sense, it is similar to > CVE-2019-6488 in impact, right? If we decide to treat this as a For x32, there is no invalid memory access. It just gives the wrong result. > security vulnerability, we need a new CVE ID because the version range > is different (bug 24155 was not fixed in the 2.29 release). > Since the wrong result from memcmp may lead to security vulnerability, we should apply for CVE. Thanks.
* H. J. Lu: > On Sat, Feb 2, 2019 at 6:57 AM Florian Weimer <fweimer@redhat.com> wrote: >> >> * H. J. Lu: >> >> > Since the size argument is unsigned. we should use unsigned Jcc >> > instructions, instead of signed to check size. >> > >> > Tested on x86-64 and x32, with and without --disable-multi-arch. >> >> Does this impact x86-64 at all (technically), consider that an object >> size larger than SSIZE_MAX would be undefined anyway? > > I don't think we will hit it on x86-64. > >> It seems that on x32, it can give incorrect results if the sign bit on >> the 64-bit register is set. In this sense, it is similar to >> CVE-2019-6488 in impact, right? If we decide to treat this as a > > For x32, there is no invalid memory access. It just gives the wrong > result. Well, that could be problematic as well. Does the comparison stop early, only checking a prefix? >> security vulnerability, we need a new CVE ID because the version range >> is different (bug 24155 was not fixed in the 2.29 release). >> > > Since the wrong result from memcmp may lead to security vulnerability, we > should apply for CVE. Sure, I'll take care of it. Thanks, Florian
On Sat, Feb 2, 2019 at 7:32 AM Florian Weimer <fweimer@redhat.com> wrote: > > * H. J. Lu: > > > On Sat, Feb 2, 2019 at 6:57 AM Florian Weimer <fweimer@redhat.com> wrote: > >> > >> * H. J. Lu: > >> > >> > Since the size argument is unsigned. we should use unsigned Jcc > >> > instructions, instead of signed to check size. > >> > > >> > Tested on x86-64 and x32, with and without --disable-multi-arch. > >> > >> Does this impact x86-64 at all (technically), consider that an object > >> size larger than SSIZE_MAX would be undefined anyway? > > > > I don't think we will hit it on x86-64. > > > >> It seems that on x32, it can give incorrect results if the sign bit on > >> the 64-bit register is set. In this sense, it is similar to > >> CVE-2019-6488 in impact, right? If we decide to treat this as a > > > > For x32, there is no invalid memory access. It just gives the wrong > > result. > > Well, that could be problematic as well. > > Does the comparison stop early, only checking a prefix? On x32, memcmp always returns 0 when the most significant bit of RDX is set since it treats size as 0, like memcmp (a, b, 0). > >> security vulnerability, we need a new CVE ID because the version range > >> is different (bug 24155 was not fixed in the 2.29 release). > >> > > > > Since the wrong result from memcmp may lead to security vulnerability, we > > should apply for CVE. > > Sure, I'll take care of it. > Thanks.
* H. J. Lu: > On Sat, Feb 2, 2019 at 7:32 AM Florian Weimer <fweimer@redhat.com> wrote: >> >> * H. J. Lu: >> >> > On Sat, Feb 2, 2019 at 6:57 AM Florian Weimer <fweimer@redhat.com> wrote: >> >> >> >> * H. J. Lu: >> >> >> >> > Since the size argument is unsigned. we should use unsigned Jcc >> >> > instructions, instead of signed to check size. >> >> > >> >> > Tested on x86-64 and x32, with and without --disable-multi-arch. >> >> >> >> Does this impact x86-64 at all (technically), consider that an object >> >> size larger than SSIZE_MAX would be undefined anyway? >> > >> > I don't think we will hit it on x86-64. >> > >> >> It seems that on x32, it can give incorrect results if the sign bit on >> >> the 64-bit register is set. In this sense, it is similar to >> >> CVE-2019-6488 in impact, right? If we decide to treat this as a >> > >> > For x32, there is no invalid memory access. It just gives the wrong >> > result. >> >> Well, that could be problematic as well. >> >> Does the comparison stop early, only checking a prefix? > > On x32, memcmp always returns 0 when the most significant bit of RDX is set > since it treats size as 0, like memcmp (a, b, 0). > >> >> security vulnerability, we need a new CVE ID because the version range >> >> is different (bug 24155 was not fixed in the 2.29 release). >> >> >> > >> > Since the wrong result from memcmp may lead to security vulnerability, we >> > should apply for CVE. >> >> Sure, I'll take care of it. MITRE has assigned CVE-2019-7309. Please mention it in the appropriate places. Thanks, Florian
diff --git a/sysdeps/x86_64/memcmp.S b/sysdeps/x86_64/memcmp.S index 1fc487caa5..1322bb3b92 100644 --- a/sysdeps/x86_64/memcmp.S +++ b/sysdeps/x86_64/memcmp.S @@ -21,14 +21,18 @@ .text ENTRY (memcmp) - test %rdx, %rdx +#ifdef __ILP32__ + /* Clear the upper 32 bits. */ + movl %edx, %edx +#endif + test %RDX_LP, %RDX_LP jz L(finz) cmpq $1, %rdx - jle L(finr1b) + jbe L(finr1b) subq %rdi, %rsi movq %rdx, %r10 cmpq $32, %r10 - jge L(gt32) + jae L(gt32) /* Handle small chunks and last block of less than 32 bytes. */ L(small): testq $1, %r10 @@ -156,7 +160,7 @@ L(A32): movq %r11, %r10 andq $-32, %r10 cmpq %r10, %rdi - jge L(mt16) + jae L(mt16) /* Pre-unroll to be ready for unrolled 64B loop. */ testq $32, %rdi jz L(A64) @@ -178,7 +182,7 @@ L(A64): movq %r11, %r10 andq $-64, %r10 cmpq %r10, %rdi - jge L(mt32) + jae L(mt32) L(A64main): movdqu (%rdi,%rsi), %xmm0 @@ -216,7 +220,7 @@ L(mt32): movq %r11, %r10 andq $-32, %r10 cmpq %r10, %rdi - jge L(mt16) + jae L(mt16) L(A32main): movdqu (%rdi,%rsi), %xmm0 @@ -254,7 +258,7 @@ L(ATR): movq %r11, %r10 andq $-32, %r10 cmpq %r10, %rdi - jge L(mt16) + jae L(mt16) testq $16, %rdi jz L(ATR32) @@ -325,7 +329,7 @@ L(ATR64main): movq %r11, %r10 andq $-32, %r10 cmpq %r10, %rdi - jge L(mt16) + jae L(mt16) L(ATR32res): movdqa (%rdi,%rsi), %xmm0 diff --git a/sysdeps/x86_64/x32/Makefile b/sysdeps/x86_64/x32/Makefile index 1557724b0c..8748956563 100644 --- a/sysdeps/x86_64/x32/Makefile +++ b/sysdeps/x86_64/x32/Makefile @@ -8,7 +8,8 @@ endif ifeq ($(subdir),string) tests += tst-size_t-memchr tst-size_t-memcmp tst-size_t-memcpy \ tst-size_t-memrchr tst-size_t-memset tst-size_t-strncasecmp \ - tst-size_t-strncmp tst-size_t-strncpy tst-size_t-strnlen + tst-size_t-strncmp tst-size_t-strncpy tst-size_t-strnlen \ + tst-size_t-memcmp-2 endif ifeq ($(subdir),wcsmbs) diff --git a/sysdeps/x86_64/x32/tst-size_t-memcmp-2.c b/sysdeps/x86_64/x32/tst-size_t-memcmp-2.c new file mode 100644 index 0000000000..d8ae1a0813 --- /dev/null +++ b/sysdeps/x86_64/x32/tst-size_t-memcmp-2.c @@ -0,0 +1,79 @@ +/* Test memcmp with size_t in the lower 32 bits of 64-bit register. + Copyright (C) 2019 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 + <http://www.gnu.org/licenses/>. */ + +#define TEST_MAIN +#ifdef WIDE +# define TEST_NAME "wmemcmp" +#else +# define TEST_NAME "memcmp" +#endif + +#include "test-size_t.h" + +#ifdef WIDE +# include <inttypes.h> +# include <wchar.h> + +# define MEMCMP wmemcmp +# define CHAR wchar_t +#else +# define MEMCMP memcmp +# define CHAR char +#endif + +IMPL (MEMCMP, 1) + +typedef int (*proto_t) (const CHAR *, const CHAR *, size_t); + +static int +__attribute__ ((noinline, noclone)) +do_memcmp (parameter_t a, parameter_t b) +{ + return CALL (&b, a.p, b.p, a.len); +} + +static int +test_main (void) +{ + test_init (); + + parameter_t dest = { { page_size / sizeof (CHAR) }, buf1 }; + parameter_t src = { { 0 }, buf2 }; + + memcpy (buf1, buf2, page_size); + + CHAR *p = (CHAR *) buf1; + p[page_size / sizeof (CHAR) - 1] = (CHAR) 1; + + int ret = 0; + FOR_EACH_IMPL (impl, 0) + { + src.fn = impl->fn; + int res = do_memcmp (dest, src); + if (res >= 0) + { + error (0, 0, "Wrong result in function %s: %i >= 0", + impl->name, res); + ret = 1; + } + } + + return ret ? EXIT_FAILURE : EXIT_SUCCESS; +} + +#include <support/test-driver.c>