Re: [PATCH 5.4 regression fix] x86/boot: Provide memzero_explicit

[Stephan Mueller <smueller@chronox.de>]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

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

[-- Attachment #2: memset_secure.c --]
[-- Type: text/x-csrc, Size: 4035 bytes --]

/*
 * 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