Messages in this thread | | | From | Stephan Mueller <> | Subject | Re: [PATCH 5.4 regression fix] x86/boot: Provide memzero_explicit | Date | Mon, 07 Oct 2019 11:34:09 +0200 |
| |
Am Montag, 7. Oktober 2019, 11:06:04 CEST schrieb Hans de Goede:
Hi Hans,
> Hi Stephan, > > On 07-10-2019 10:59, Stephan Mueller wrote: > > Am Montag, 7. Oktober 2019, 10:55:01 CEST schrieb Hans de Goede: > > > > Hi Hans, > > > >> The purgatory code now uses the shared lib/crypto/sha256.c sha256 > >> implementation. This needs memzero_explicit, implement this. > >> > >> Reported-by: Arvind Sankar <nivedita@alum.mit.edu> > >> Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get > >> input, memzero_explicit") Signed-off-by: Hans de Goede > >> <hdegoede@redhat.com> > >> --- > >> > >> arch/x86/boot/compressed/string.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/arch/x86/boot/compressed/string.c > >> b/arch/x86/boot/compressed/string.c index 81fc1eaa3229..511332e279fe > >> 100644 > >> --- a/arch/x86/boot/compressed/string.c > >> +++ b/arch/x86/boot/compressed/string.c > >> @@ -50,6 +50,11 @@ void *memset(void *s, int c, size_t n) > >> > >> return s; > >> > >> } > >> > >> +void memzero_explicit(void *s, size_t count) > >> +{ > >> + memset(s, 0, count); > > > > May I ask how it is guaranteed that this memset is not optimized out by > > the > > compiler, e.g. for stack variables? > > The function and the caller live in different compile units, so unless > LTO is used this cannot happen.
Agreed in this case.
I would just be worried that this memzero_explicit implementation is assumed to be protected against optimization when used elsewhere since other implementations of memzero_explicit are provided with the goal to be protected against optimizations. > > Also note that the previous purgatory private (vs shared) sha256 > implementation had: > > /* Zeroize sensitive information. */ > memset(sctx, 0, sizeof(*sctx)); > > In the place where the new shared 256 code uses memzero_explicit() and the > new shared sha256 code is the only user of the > arch/x86/boot/compressed/string.c memzero_explicit() implementation. > > With that all said I'm open to suggestions for improving this.
What speaks against the common memzero_explicit implementation? If you cannot use it, what about adding a barrier in the memzero_explicit implementation? Or what about adding some compiler magic as attached to this email?
> > Regards, > > Hans
Ciao Stephan/* * Copyright (C) 2015, Stephan Mueller <smueller@chronox.de> * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions * are met: * 1. Redistributions of source code must retain the above copyright * notice, and the entire permission notice in its entirety, * including the disclaimer of warranties. * 2. Redistributions in binary form must reproduce the above copyright * notice, this list of conditions and the following disclaimer in the * documentation and/or other materials provided with the distribution. * 3. The name of the author may not be used to endorse or promote * products derived from this software without specific prior * written permission. * * ALTERNATIVELY, this product may be distributed under the terms of * the GNU General Public License, in which case the provisions of the GPL2 * are required INSTEAD OF the above restrictions. (This clause is * necessary due to a potential bad interaction between the GPL and * the restrictions contained in a BSD-style copyright.) * * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE, ALL OF * WHICH ARE HEREBY DISCLAIMED. IN NO EVENT SHALL THE AUTHOR BE * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT * OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE * USE OF THIS SOFTWARE, EVEN IF NOT ADVISED OF THE POSSIBILITY OF SUCH * DAMAGE. */
#include <string.h>
/* * Tested following code: * * (1) __asm__ __volatile__("" : "=r" (s) : "0" (s)); * (2) __asm__ __volatile__("": : :"memory"); * (3) __asm__ __volatile__("" : "=r" (s) : "0" (s) : "memory"); * (4) __asm__ __volatile__("" : : "r" (s) : "memory"); * * Requred result: * * gcc -O3: objdump -d shows the following: * * 0000000000400440 <main>: * ... * 400469: 48 c7 04 24 00 00 00 movq $0x0,(%rsp) * 400470: 00 * 400471: 48 c7 44 24 08 00 00 movq $0x0,0x8(%rsp) * 400478: 00 00 * 40047a: c7 44 24 10 00 00 00 movl $0x0,0x10(%rsp) * 400481: 00 * * clang -O3: objdump -d shows the following: * * 0000000000400590 <main>: * ... * 4005c3: c7 44 24 10 00 00 00 movl $0x0,0x10(%rsp) * 4005ca: 00 * * * Test results: * * The following table marks an X when the aforementioned movq/movl code is * present (or an invocation of memset@plt) in the object code * (i.e. the code we want). Contrary, the table marks - where the code is not * present (i.e. the code we do not want): * * | BARRIER | (1) | (2) | (3) | (4) * ---------+----------+ | | | * Compiler | | | | | * =========+==========+======================= * | | | | * gcc -O0 | X | X | X | X * | | | | * gcc -O2 | - | X | X | X * | | | | * gcc -O3 | - | X | X | X * | | | | * clang -00 | X | X | X | X * | | | | * clang -02 | X | - | X | X * | | | | * clang -03 | - | - | X | X */
static inline void memset_secure(void *s, int c, size_t n) { memset(s, c, n); __asm__ __volatile__("" : : "r" (s) : "memory"); }
#if 0 #include <stdio.h>
int main(int argc, char *argv[]) { char buf[20];
snprintf(buf, sizeof(buf) - 1, "test"); printf("%s\n", buf);
memset_secure(buf, 0, sizeof(buf)); return 0; } #endif
| |