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

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

[Hans de Goede]

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);
+}
+
 void *memmove(void *dest, const void *src, size_t n)
 {
 	unsigned char *d = dest;
-- 
2.23.0

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

[Stephan Mueller]

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?
> +}
> +
>  void *memmove(void *dest, const void *src, size_t n)
>  {
>  	unsigned char *d = dest;



Ciao
Stephan

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

[Hans de Goede]

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.

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.

Regards,

Hans

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

[Stephan Mueller]

[-- 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

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

[Hans de Goede]

Hi Stephan,

On 07-10-2019 11:34, Stephan Mueller wrote:
> 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?

Nothing, but the purgatory is a standalone binary which runs between
2 kernels when doing kexec so it cannot use the function from lib/string.c
since it is not linked against the lib/string.o object.

> 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?

Since the purgatory code is running in a somewhat limited environment,
with not all standard headers / macros available I was afraid that the
barrier_data() from the lib/string.c implementation would not work, so
I left it out. In hindsight I should have really given it a try first as
it seems to compile fine and there are no missing symbols in
arch/x86/purgatory/purgatory.ro when using it.

So I will send out a new version with the barrier_data() added making
the arch/x86/boot/compressed/string.c implementation identical to the
lib/string.c one.

Regards,

Hans

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

[Arvind Sankar]

On Mon, Oct 07, 2019 at 11:34:09AM +0200, Stephan Mueller wrote:
> 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?
> 

The common definition I think is the same as your attachment, i.e.
memset followed by barrier_data(). I don't think there is any reason not
to just copy that definition?

Alternatively, could the common definition not be made an inline or
macro? or is there a risk that could introduce unsafe optimizations to
eliminate it?

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

[Arvind Sankar]

On Mon, Oct 07, 2019 at 03:00:51PM +0200, Hans de Goede wrote:
> Hi Stephan,
> 
> On 07-10-2019 11:34, Stephan Mueller wrote:
> > 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?
> 
> Nothing, but the purgatory is a standalone binary which runs between
> 2 kernels when doing kexec so it cannot use the function from lib/string.c
> since it is not linked against the lib/string.o object.
> 
> > 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?
> 
> Since the purgatory code is running in a somewhat limited environment,
> with not all standard headers / macros available I was afraid that the
> barrier_data() from the lib/string.c implementation would not work, so
> I left it out. In hindsight I should have really given it a try first as
> it seems to compile fine and there are no missing symbols in
> arch/x86/purgatory/purgatory.ro when using it.
> 
> So I will send out a new version with the barrier_data() added making
> the arch/x86/boot/compressed/string.c implementation identical to the
> lib/string.c one.
> 
> Regards,
> 
> Hans
> 

I think we also need a fix for at least s390 right? That also has sha256
verification and would presumably have the same issue with undefined
memzero_explicit? powerpc does not seem to do sha256 verification
afaict.