From patchwork Mon Feb 4 01:03:38 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "H.J. Lu" X-Patchwork-Id: 31289 Received: (qmail 29737 invoked by alias); 4 Feb 2019 01:03:45 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 29722 invoked by uid 89); 4 Feb 2019 01:03:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-pl1-f194.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=UppMHCcJQiLxbVD5k9LkW8Gz2S7FP8ioirRENx97cxI=; b=O33ckGByM60wycwAQvUory4uRYGFRdG554F/eobFi7BnelhftQuH5ZMjS4wk9BGRAz 8NZdLGkziEICbasn/ujpsgB6sOWl9delNaVobd+qSqBv30Tch6abUj5mV6JG2uf+E+Ly Hyq7L9NLOj8AxPzeE+v8Zpid7gY7cEb5edvK0chnJ0z1mgGiJceCLArTxRikCYJddVVG /8L/6763RUFFL4QLeCJ/vhqzP/Fv57XwD4YAZGZx2jn9DH6ftNDFxURGo++WWZ2BEEGm zmp5ytENw6Yu5qv4pz78LQ+EM5Rouzi+bu35YNS1QWlAfk1QP2wfvehQIxCV88TRN5aD b32A== Return-Path: Date: Sun, 3 Feb 2019 17:03:38 -0800 From: "H.J. Lu" To: Florian Weimer Cc: GNU C Library Subject: [PATCH] x86-64 memcmp: Use unsigned Jcc instructions on size [BZ #24155] Message-ID: <20190204010338.GA4441@gmail.com> References: <20190202142723.7730-1-hjl.tools@gmail.com> <87munew85i.fsf@oldenburg2.str.redhat.com> <875zu2w6hl.fsf@oldenburg2.str.redhat.com> <87bm3t6zkw.fsf@oldenburg2.str.redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87bm3t6zkw.fsf@oldenburg2.str.redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) On Sun, Feb 03, 2019 at 09:33:35AM +0100, Florian Weimer wrote: > * H. J. Lu: > > > On Sat, Feb 2, 2019 at 7:32 AM Florian Weimer wrote: > >> > >> * H. J. Lu: > >> > >> > On Sat, Feb 2, 2019 at 6:57 AM Florian Weimer 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. > This is the patch I am going to check in tomorow. Thanks. H.J. --- 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. [BZ #24155] CVE-2019-7309 * NEWS: Updated for CVE-2019-7309. * sysdeps/x86_64/memcmp.S: Use RDX_LP for size. Clear the upper 32 bits of RDX register for x32. Use unsigned Jcc instructions, instead of signed. * sysdeps/x86_64/x32/Makefile (tests): Add tst-size_t-memcmp-2. * sysdeps/x86_64/x32/tst-size_t-memcmp-2.c: New test. --- NEWS | 8 ++- sysdeps/x86_64/memcmp.S | 20 +++--- sysdeps/x86_64/x32/Makefile | 3 +- sysdeps/x86_64/x32/tst-size_t-memcmp-2.c | 79 ++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 10 deletions(-) create mode 100644 sysdeps/x86_64/x32/tst-size_t-memcmp-2.c diff --git a/NEWS b/NEWS index 5cf568aed9..bc0b2bc3df 100644 --- a/NEWS +++ b/NEWS @@ -24,7 +24,13 @@ Changes to build and runtime requirements: Security related changes: - [Add security related changes here] + CVE-2019-7309: x86-64 memcmp used signed Jcc instructions to check + size. For x86-64, memcmp on an object size larger than SSIZE_MAX + has undefined behavior. On x32, the size_t argument may be passed + in the lower 32 bits of the 64-bit RDX register with non-zero upper + 32 bits. When it happened with the sign bit of RDX register set, + memcmp gave the wrong result since it treated the size argument as + zero. Reported by H.J. Lu. The following bugs are resolved with this release: 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 + . */ + +#define TEST_MAIN +#ifdef WIDE +# define TEST_NAME "wmemcmp" +#else +# define TEST_NAME "memcmp" +#endif + +#include "test-size_t.h" + +#ifdef WIDE +# include +# include + +# 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