From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x32.google.com (mail-oa1-x32.google.com [IPv6:2001:4860:4864:20::32]) by sourceware.org (Postfix) with ESMTPS id A2BB638537AB for ; Wed, 14 Dec 2022 16:08:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org A2BB638537AB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-oa1-x32.google.com with SMTP id 586e51a60fabf-143ffc8c2b2so17182940fac.2 for ; Wed, 14 Dec 2022 08:08:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=AAUoxL6kMzrKmhrK1tdOTXMtL8fzXNMnYdW8sEc3wHI=; b=F5zpiJOWoH7r8u2xMTcWJB8HiK9LAgzBZjS3nvOQzuJqAnvqnhntLMH9a1UELqBAxV eZgD5sa1Zn4rNPJ3KpCarRcGFNAQ4kIHQucUJV3bg54DVlEu1rg//AxHKMH4LtPXG/d8 ksBq34n9OgmCD3k1H/25GojNND3bO7y2pK2NGUAh2XE6CYdeQGnDZkQtKgHycaYye+SP KiYklIQYIupmUvVgdzClx89vvF0vvagcG7HfczbWBIJgxBArPcukAoj8+bj4wll/0CBp R7XCZSAvaI+VgQ6Fb0U++DQUWw0T6N7cspGEeRxcSeemQExF4C1TTm6Bb1dM3HYoPQF1 YKZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=AAUoxL6kMzrKmhrK1tdOTXMtL8fzXNMnYdW8sEc3wHI=; b=kH/NkE+IJz4CKAqPO1uTiSVe+b1LuJB9ck+kg0SOHGj0pOjQzZWYdw+f4sb9LwznpT jzre1FoFUIUmhyTbdO6jxiERjqZgkphxjlxVfhRhuaLb/+MtOptwTGDlh0CEW66xvXaO UlcPgpSvCLO5w2a4E8Bfwl7o/xlF+iEmjxpGsJIrDjPuEF9S9dh5EymlEsauYPvpuk9L RKrTKDWPWDEgOASCC4UeczXVGeplsINd/wyePif6YS79Hne+TpHbaTKaGd09LnV1FZJu lQLRwvHhRwBv/bB0pAKy1utf/AkArQp/q7IwoDESVbT7etRxD0KJwuXL/s8wA/Ga4V3W fGHA== X-Gm-Message-State: AFqh2krSQlXV7F09uYaK0196gJExWe1cfBaatjxI2V1arbpsLYV3LPwP z6SB0vRclrx9l2qGD9VgKgl38H3bqtPyUkU6FJtMXf2s X-Google-Smtp-Source: AMrXdXtmwdisb9bNUJNu599fZ3p/9cMlrARp2CoJOG4+6ZWY/hGaZ7x5JSwzP8rpCmoeg2Ugpzl5gk1+gPrAU7dwEK0= X-Received: by 2002:a05:6870:8087:b0:148:2d85:b411 with SMTP id q7-20020a056870808700b001482d85b411mr303791oab.266.1671034085937; Wed, 14 Dec 2022 08:08:05 -0800 (PST) MIME-Version: 1.0 References: <20221214001147.2814047-1-goldstein.w.n@gmail.com> In-Reply-To: <20221214001147.2814047-1-goldstein.w.n@gmail.com> From: "H.J. Lu" Date: Wed, 14 Dec 2022 08:07:29 -0800 Message-ID: Subject: Re: [PATCH v1] x86: Prevent SIG11 in memcmp-sse2 when data is concurrently modified [BZ #29863] To: Noah Goldstein Cc: libc-alpha@sourceware.org, carlos@systemhalted.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3022.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, Dec 13, 2022 at 4:12 PM Noah Goldstein wrote: > > In the case of INCORRECT usage of `memcmp(a, b, N)` where `a` and `b` > are concurrently modified as `memcmp` runs, there can be a SIG11 in > `L(ret_nonzero_vec_end_0)` because the sequential logic assumes > that `(rdx - 32 + rax)` is a positive 32-bit integer. > > To be clear, this "fix" does not mean this usage of `memcmp` is > supported. `memcmp` is incorrect when the values of `a` and/or `b` > are modified while its running, and that incorrectness may manifest > itself as a SIG-11. That being said, if we can make the results > less dramatic with no cost to regular uses cases, there is no harm > in doing so. > > The fix replaces a 32-bit `addl %edx, %eax` with the 64-bit variant > `addq %rdx, %rax`. The 1-extra byte of code size from using the > 64-bit instruction doesn't contribute to overall code size as the > next target is aligned and has multiple bytes of `nop` padding > before it. As well all the logic between the add and `ret` still > fits in the same fetch block, so the cost of this change is > basically zero. > > The sequential logic makes the assume behind the following code: > ``` > /* > * rsi = a > * rdi = b > * rdx = len - 32 > */ > /* cmp a[0:15] and b[0:15]. Since length is known to be [17, 32] > in this case, this check is also assume to cover a[0:(31 - len)] > and b[0:(31 - len)]. */ > movups (%rsi), %xmm0 > movups (%rdi), %xmm1 > PCMPEQ %xmm0, %xmm1 > pmovmskb %xmm1, %eax > subl %ecx, %eax > jnz L(END_NEQ) > > /* cmp a[len-16:len-1] and b[len-16:len-1]. */ > movups 16(%rsi, %rdx), %xmm0 > movups 16(%rdi, %rdx), %xmm1 > PCMPEQ %xmm0, %xmm1 > pmovmskb %xmm1, %eax > subl %ecx, %eax > jnz L(END_NEQ2) > ret > > L(END2): > /* Position first mismatch. */ > bsfl %eax, %eax > > /* BUG IS FROM THIS. The sequential version is able to assume this > value is a positive 32-bit value because first check included > bytes in range a[0:(31 - len)], b[0:(31 - len)] so `eax` must be > greater than `31 - len` so the minimum value of `edx` + `eax` is > `(len - 32) + (32 - len) >= 0`. In the concurrent case, however, > `a` or `b` could have been changed so a mismatch in `eax` less or > equal than `(31 - len)` is possible (the new low bound in `(16 - > len)`. This can result in a negative 32-bit signed integer, which > when non-sign extended to 64-bits is a random large value out of > bounds. */ > addl %edx, %eax > > /* Crash here because 32-bit negative number in `eax` non-sign > extends to out of bounds 64-bit offset. */ > movzbl 16(%rdi, %rax), %ecx > movzbl 16(%rsi, %rax), %eax > ``` > > This fix is quite simple, just make the `addl %edx, %eax` 64 bit (i.e > `addq %rdx, %rax`). This prevent the 32-bit non-sign extension > and since `eax` still a low bound of `16 - len` the `rdx + rax` > is bound by `(len - 32) - (16 - len) >= -16`. Since we have a > fixed offset of `16` in the memory access this must be inbounds. > --- > sysdeps/x86_64/multiarch/memcmp-sse2.S | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sysdeps/x86_64/multiarch/memcmp-sse2.S b/sysdeps/x86_64/multiarch/memcmp-sse2.S > index afd450d020..34e60e567d 100644 > --- a/sysdeps/x86_64/multiarch/memcmp-sse2.S > +++ b/sysdeps/x86_64/multiarch/memcmp-sse2.S > @@ -308,7 +308,7 @@ L(ret_nonzero_vec_end_0): > setg %dl > leal -1(%rdx, %rdx), %eax > # else > - addl %edx, %eax > + addq %rdx, %rax Please add some comments here and also include the testcase. > movzbl (VEC_SIZE * -1 + SIZE_OFFSET)(%rsi, %rax), %ecx > movzbl (VEC_SIZE * -1 + SIZE_OFFSET)(%rdi, %rax), %eax > subl %ecx, %eax > -- > 2.34.1 > Thanks. -- H.J.