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

[PATCH v2 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>
---
Changes in v2:
- Add barrier_data() call after the memset, making the function really
  explicit. Using barrier_data() works fine in the purgatory (build)
  environment.
---
 arch/x86/boot/compressed/string.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 81fc1eaa3229..654a7164a702 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -50,6 +50,12 @@ void *memset(void *s, int c, size_t n)
 	return s;
 }
 
+void memzero_explicit(void *s, size_t count)
+{
+	memset(s, 0, count);
+	barrier_data(s);
+}
+
 void *memmove(void *dest, const void *src, size_t n)
 {
 	unsigned char *d = dest;
-- 
2.23.0

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

[Ingo Molnar]

* Hans de Goede <hdegoede@redhat.com> wrote:

> 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>
> ---
> Changes in v2:
> - Add barrier_data() call after the memset, making the function really
>   explicit. Using barrier_data() works fine in the purgatory (build)
>   environment.
> ---
>  arch/x86/boot/compressed/string.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
> index 81fc1eaa3229..654a7164a702 100644
> --- a/arch/x86/boot/compressed/string.c
> +++ b/arch/x86/boot/compressed/string.c
> @@ -50,6 +50,12 @@ void *memset(void *s, int c, size_t n)
>  	return s;
>  }
>  
> +void memzero_explicit(void *s, size_t count)
> +{
> +	memset(s, 0, count);
> +	barrier_data(s);
> +}

So the barrier_data() is only there to keep LTO from optimizing out the 
seemingly unused function?

Is there no canonical way to do that, instead of a seemingly obscure 
barrier_data() call - which would require a comment at minimum.

We do have $(DISABLE_LTO), not sure whether it's applicable here though.

Thanks,

	Ingo

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

[Hans de Goede]

Hi,

On 07-10-2019 16:00, Ingo Molnar wrote:
> 
> * Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> 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>
>> ---
>> Changes in v2:
>> - Add barrier_data() call after the memset, making the function really
>>    explicit. Using barrier_data() works fine in the purgatory (build)
>>    environment.
>> ---
>>   arch/x86/boot/compressed/string.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
>> index 81fc1eaa3229..654a7164a702 100644
>> --- a/arch/x86/boot/compressed/string.c
>> +++ b/arch/x86/boot/compressed/string.c
>> @@ -50,6 +50,12 @@ void *memset(void *s, int c, size_t n)
>>   	return s;
>>   }
>>   
>> +void memzero_explicit(void *s, size_t count)
>> +{
>> +	memset(s, 0, count);
>> +	barrier_data(s);
>> +}
> 
> So the barrier_data() is only there to keep LTO from optimizing out the
> seemingly unused function?

I believe that Stephan Mueller (who suggested adding the barrier)
was also worried about people using this as an example for other
"explicit" functions which actually might get inlined.

This is not so much about protecting against LTO as it is against
protecting against inlining, which in this case boils down to the
same thing. Also this change makes the arch/x86/boot/compressed/string.c
and lib/string.c versions identical which seems like a good thing to me
(except for the code duplication part of it).

But I agree a comment would be good, how about:

void memzero_explicit(void *s, size_t count)
{
	memset(s, 0, count);
	/* Avoid the memset getting optimized away if we ever get inlined */
	barrier_data(s);
}

?

Regards,

Hans

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

[Ingo Molnar]

* Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 07-10-2019 16:00, Ingo Molnar wrote:
> > 
> > * Hans de Goede <hdegoede@redhat.com> wrote:
> > 
> > > 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>
> > > ---
> > > Changes in v2:
> > > - Add barrier_data() call after the memset, making the function really
> > >    explicit. Using barrier_data() works fine in the purgatory (build)
> > >    environment.
> > > ---
> > >   arch/x86/boot/compressed/string.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
> > > index 81fc1eaa3229..654a7164a702 100644
> > > --- a/arch/x86/boot/compressed/string.c
> > > +++ b/arch/x86/boot/compressed/string.c
> > > @@ -50,6 +50,12 @@ void *memset(void *s, int c, size_t n)
> > >   	return s;
> > >   }
> > > +void memzero_explicit(void *s, size_t count)
> > > +{
> > > +	memset(s, 0, count);
> > > +	barrier_data(s);
> > > +}
> > 
> > So the barrier_data() is only there to keep LTO from optimizing out the
> > seemingly unused function?
> 
> I believe that Stephan Mueller (who suggested adding the barrier)
> was also worried about people using this as an example for other
> "explicit" functions which actually might get inlined.
> 
> This is not so much about protecting against LTO as it is against
> protecting against inlining, which in this case boils down to the
> same thing. Also this change makes the arch/x86/boot/compressed/string.c
> and lib/string.c versions identical which seems like a good thing to me
> (except for the code duplication part of it).
> 
> But I agree a comment would be good, how about:
> 
> void memzero_explicit(void *s, size_t count)
> {
> 	memset(s, 0, count);
> 	/* Avoid the memset getting optimized away if we ever get inlined */
> 	barrier_data(s);
> }

Well, the standard construct for preventing inlining would be 'noinline', 
right? Any reason that wouldn't work?

Thanks,

	Ingo

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

[Hans de Goede]

Hi,

On 07-10-2019 16:22, Ingo Molnar wrote:
> 
> * Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 07-10-2019 16:00, Ingo Molnar wrote:
>>>
>>> * Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>>> 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>
>>>> ---
>>>> Changes in v2:
>>>> - Add barrier_data() call after the memset, making the function really
>>>>     explicit. Using barrier_data() works fine in the purgatory (build)
>>>>     environment.
>>>> ---
>>>>    arch/x86/boot/compressed/string.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
>>>> index 81fc1eaa3229..654a7164a702 100644
>>>> --- a/arch/x86/boot/compressed/string.c
>>>> +++ b/arch/x86/boot/compressed/string.c
>>>> @@ -50,6 +50,12 @@ void *memset(void *s, int c, size_t n)
>>>>    	return s;
>>>>    }
>>>> +void memzero_explicit(void *s, size_t count)
>>>> +{
>>>> +	memset(s, 0, count);
>>>> +	barrier_data(s);
>>>> +}
>>>
>>> So the barrier_data() is only there to keep LTO from optimizing out the
>>> seemingly unused function?
>>
>> I believe that Stephan Mueller (who suggested adding the barrier)
>> was also worried about people using this as an example for other
>> "explicit" functions which actually might get inlined.
>>
>> This is not so much about protecting against LTO as it is against
>> protecting against inlining, which in this case boils down to the
>> same thing. Also this change makes the arch/x86/boot/compressed/string.c
>> and lib/string.c versions identical which seems like a good thing to me
>> (except for the code duplication part of it).
>>
>> But I agree a comment would be good, how about:
>>
>> void memzero_explicit(void *s, size_t count)
>> {
>> 	memset(s, 0, count);
>> 	/* Avoid the memset getting optimized away if we ever get inlined */
>> 	barrier_data(s);
>> }
> 
> Well, the standard construct for preventing inlining would be 'noinline',
> right? Any reason that wouldn't work?

Good question. I guess the worry is that modern compilers are getting
more aggressive with optimizing and then even if not inlined if the
function gets compiled in the same scope, then the compiler might
still notice it is only every writing to the memory passed in; and
then optimize it away of the write happens to memory which lifetime
ends immediately afterwards. I mean removing the call is not inlining,
so compiler developers might decide that that is still fine to do.

IMHO with trickycode like this is is best to just use the proven
version from lib/string.c

I guess I made the comment to specific though, so how about:

void memzero_explicit(void *s, size_t count)
{
	memset(s, 0, count);
	/* Tell the compiler to never remove / optimize away the memset */
	barrier_data(s);
}

Regards,

Hans

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

[Ingo Molnar]

* Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 07-10-2019 16:22, Ingo Molnar wrote:
> > 
> > * Hans de Goede <hdegoede@redhat.com> wrote:
> > 
> > > Hi,
> > > 
> > > On 07-10-2019 16:00, Ingo Molnar wrote:
> > > > 
> > > > * Hans de Goede <hdegoede@redhat.com> wrote:
> > > > 
> > > > > 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>
> > > > > ---
> > > > > Changes in v2:
> > > > > - Add barrier_data() call after the memset, making the function really
> > > > >     explicit. Using barrier_data() works fine in the purgatory (build)
> > > > >     environment.
> > > > > ---
> > > > >    arch/x86/boot/compressed/string.c | 6 ++++++
> > > > >    1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
> > > > > index 81fc1eaa3229..654a7164a702 100644
> > > > > --- a/arch/x86/boot/compressed/string.c
> > > > > +++ b/arch/x86/boot/compressed/string.c
> > > > > @@ -50,6 +50,12 @@ void *memset(void *s, int c, size_t n)
> > > > >    	return s;
> > > > >    }
> > > > > +void memzero_explicit(void *s, size_t count)
> > > > > +{
> > > > > +	memset(s, 0, count);
> > > > > +	barrier_data(s);
> > > > > +}
> > > > 
> > > > So the barrier_data() is only there to keep LTO from optimizing out the
> > > > seemingly unused function?
> > > 
> > > I believe that Stephan Mueller (who suggested adding the barrier)
> > > was also worried about people using this as an example for other
> > > "explicit" functions which actually might get inlined.
> > > 
> > > This is not so much about protecting against LTO as it is against
> > > protecting against inlining, which in this case boils down to the
> > > same thing. Also this change makes the arch/x86/boot/compressed/string.c
> > > and lib/string.c versions identical which seems like a good thing to me
> > > (except for the code duplication part of it).
> > > 
> > > But I agree a comment would be good, how about:
> > > 
> > > void memzero_explicit(void *s, size_t count)
> > > {
> > > 	memset(s, 0, count);
> > > 	/* Avoid the memset getting optimized away if we ever get inlined */
> > > 	barrier_data(s);
> > > }
> > 
> > Well, the standard construct for preventing inlining would be 'noinline',
> > right? Any reason that wouldn't work?
> 
> Good question. I guess the worry is that modern compilers are getting
> more aggressive with optimizing and then even if not inlined if the
> function gets compiled in the same scope, then the compiler might
> still notice it is only every writing to the memory passed in; and
> then optimize it away of the write happens to memory which lifetime
> ends immediately afterwards. I mean removing the call is not inlining,
> so compiler developers might decide that that is still fine to do.
> 
> IMHO with trickycode like this is is best to just use the proven
> version from lib/string.c
> 
> I guess I made the comment to specific though, so how about:
> 
> void memzero_explicit(void *s, size_t count)
> {
> 	memset(s, 0, count);
> 	/* Tell the compiler to never remove / optimize away the memset */
> 	barrier_data(s);
> }

Ok, I guess this will work.

Thanks,

	Ingo

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

[Arvind Sankar]

On Mon, Oct 07, 2019 at 04:46:00PM +0200, Ingo Molnar wrote:
> 
> * Hans de Goede <hdegoede@redhat.com> wrote:
> 
> > Hi,
> > 
> > On 07-10-2019 16:22, Ingo Molnar wrote:
> > > 
> > > * Hans de Goede <hdegoede@redhat.com> wrote:
> > > 
> > > > Hi,
> > > > 
> > > > On 07-10-2019 16:00, Ingo Molnar wrote:
> > > > > 
> > > > > * Hans de Goede <hdegoede@redhat.com> wrote:
> > > > > 
> > > > > > 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>
> > > > > > ---
> > > > > > Changes in v2:
> > > > > > - Add barrier_data() call after the memset, making the function really
> > > > > >     explicit. Using barrier_data() works fine in the purgatory (build)
> > > > > >     environment.
> > > > > > ---
> > > > > >    arch/x86/boot/compressed/string.c | 6 ++++++
> > > > > >    1 file changed, 6 insertions(+)
> > > > > > 
> > > > > > diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
> > > > > > index 81fc1eaa3229..654a7164a702 100644
> > > > > > --- a/arch/x86/boot/compressed/string.c
> > > > > > +++ b/arch/x86/boot/compressed/string.c
> > > > > > @@ -50,6 +50,12 @@ void *memset(void *s, int c, size_t n)
> > > > > >    	return s;
> > > > > >    }
> > > > > > +void memzero_explicit(void *s, size_t count)
> > > > > > +{
> > > > > > +	memset(s, 0, count);
> > > > > > +	barrier_data(s);
> > > > > > +}
> > > > > 
> > > > > So the barrier_data() is only there to keep LTO from optimizing out the
> > > > > seemingly unused function?
> > > > 
> > > > I believe that Stephan Mueller (who suggested adding the barrier)
> > > > was also worried about people using this as an example for other
> > > > "explicit" functions which actually might get inlined.
> > > > 
> > > > This is not so much about protecting against LTO as it is against
> > > > protecting against inlining, which in this case boils down to the
> > > > same thing. Also this change makes the arch/x86/boot/compressed/string.c
> > > > and lib/string.c versions identical which seems like a good thing to me
> > > > (except for the code duplication part of it).
> > > > 
> > > > But I agree a comment would be good, how about:
> > > > 
> > > > void memzero_explicit(void *s, size_t count)
> > > > {
> > > > 	memset(s, 0, count);
> > > > 	/* Avoid the memset getting optimized away if we ever get inlined */
> > > > 	barrier_data(s);
> > > > }
> > > 
> > > Well, the standard construct for preventing inlining would be 'noinline',
> > > right? Any reason that wouldn't work?
> > 
> > Good question. I guess the worry is that modern compilers are getting
> > more aggressive with optimizing and then even if not inlined if the
> > function gets compiled in the same scope, then the compiler might
> > still notice it is only every writing to the memory passed in; and
> > then optimize it away of the write happens to memory which lifetime
> > ends immediately afterwards. I mean removing the call is not inlining,
> > so compiler developers might decide that that is still fine to do.
> > 
> > IMHO with trickycode like this is is best to just use the proven
> > version from lib/string.c
> > 
> > I guess I made the comment to specific though, so how about:
> > 
> > void memzero_explicit(void *s, size_t count)
> > {
> > 	memset(s, 0, count);
> > 	/* Tell the compiler to never remove / optimize away the memset */
> > 	barrier_data(s);
> > }
> 
> Ok, I guess this will work.
> 
> Thanks,
> 
> 	Ingo

With the barrier in there, is there any reason to *not* inline the
function? barrier_data() is an asm statement that tells the compiler
that the asm uses the memory that was set to zero, thus preventing it
from removing the memset even if nothing else uses that memory later. A
more detailed comment is there in compiler-gcc.h. I can't see why it
wouldn't work even if it were inlined.

If the function can indeed be inlined, we could just make the common
implementation a macro and avoid duplicating it? As mentioned in another
mail, we otherwise will likely need another duplicate implementation for
arch/s390/purgatory as well.

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

[Ingo Molnar]

* Arvind Sankar <nivedita@alum.mit.edu> wrote:

> With the barrier in there, is there any reason to *not* inline the
> function? barrier_data() is an asm statement that tells the compiler
> that the asm uses the memory that was set to zero, thus preventing it
> from removing the memset even if nothing else uses that memory later. A
> more detailed comment is there in compiler-gcc.h. I can't see why it
> wouldn't work even if it were inlined.
> 
> If the function can indeed be inlined, we could just make the common
> implementation a macro and avoid duplicating it? As mentioned in another
> mail, we otherwise will likely need another duplicate implementation for
> arch/s390/purgatory as well.

I suspect macro would be justified in this case. Mind sending a v3 patch 
to demonstrate how it would all look like?

I'll zap v2 if the macro solution looks better.

Thanks,

	Ingo

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

[Arvind Sankar]

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

On Mon, Oct 07, 2019 at 05:40:07PM +0200, Ingo Molnar wrote:
> 
> * Arvind Sankar <nivedita@alum.mit.edu> wrote:
> 
> > With the barrier in there, is there any reason to *not* inline the
> > function? barrier_data() is an asm statement that tells the compiler
> > that the asm uses the memory that was set to zero, thus preventing it
> > from removing the memset even if nothing else uses that memory later. A
> > more detailed comment is there in compiler-gcc.h. I can't see why it
> > wouldn't work even if it were inlined.
> > 
> > If the function can indeed be inlined, we could just make the common
> > implementation a macro and avoid duplicating it? As mentioned in another
> > mail, we otherwise will likely need another duplicate implementation for
> > arch/s390/purgatory as well.
> 
> I suspect macro would be justified in this case. Mind sending a v3 patch 
> to demonstrate how it would all look like?
> 
> I'll zap v2 if the macro solution looks better.
> 
> Thanks,
> 
> 	Ingo

Patch attached to turn memzero_explicit into inline function.

[-- Attachment #2: 0001-lib-string-make-memzero_explicit-inline-instead-of-e.patch --]
[-- Type: text/x-diff, Size: 2880 bytes --]

From 25834b8040eff72478489be0bd8a2ff549af7f94 Mon Sep 17 00:00:00 2001
From: Arvind Sankar <nivedita@alum.mit.edu>
Date: Mon, 7 Oct 2019 14:34:24 -0400
Subject: [PATCH] lib/string: make memzero_explicit inline instead of external

With the use of the barrier implied by barrier_data(), there is no need
for memzero_explicit to be extern. Making it inline saves the overhead
of a function call, and allows the code to be reused in arch/*/purgatory
without having to duplicate the implementation.

Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 include/linux/string.h | 21 ++++++++++++++++++++-
 lib/string.c           | 21 ---------------------
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index b2f9df7f0761..b6ccdc2c7f02 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -227,7 +227,26 @@ static inline bool strstarts(const char *str, const char *prefix)
 }
 
 size_t memweight(const void *ptr, size_t bytes);
-void memzero_explicit(void *s, size_t count);
+
+/**
+ * memzero_explicit - Fill a region of memory (e.g. sensitive
+ *		      keying data) with 0s.
+ * @s: Pointer to the start of the area.
+ * @count: The size of the area.
+ *
+ * Note: usually using memset() is just fine (!), but in cases
+ * where clearing out _local_ data at the end of a scope is
+ * necessary, memzero_explicit() should be used instead in
+ * order to prevent the compiler from optimising away zeroing.
+ *
+ * memzero_explicit() doesn't need an arch-specific version as
+ * it just invokes the one of memset() implicitly.
+ */
+static inline void memzero_explicit(void *s, size_t count)
+{
+	memset(s, 0, count);
+	barrier_data(s);
+}
 
 /**
  * kbasename - return the last part of a pathname.
diff --git a/lib/string.c b/lib/string.c
index cd7a10c19210..08ec58cc673b 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -748,27 +748,6 @@ void *memset(void *s, int c, size_t count)
 EXPORT_SYMBOL(memset);
 #endif
 
-/**
- * memzero_explicit - Fill a region of memory (e.g. sensitive
- *		      keying data) with 0s.
- * @s: Pointer to the start of the area.
- * @count: The size of the area.
- *
- * Note: usually using memset() is just fine (!), but in cases
- * where clearing out _local_ data at the end of a scope is
- * necessary, memzero_explicit() should be used instead in
- * order to prevent the compiler from optimising away zeroing.
- *
- * memzero_explicit() doesn't need an arch-specific version as
- * it just invokes the one of memset() implicitly.
- */
-void memzero_explicit(void *s, size_t count)
-{
-	memset(s, 0, count);
-	barrier_data(s);
-}
-EXPORT_SYMBOL(memzero_explicit);
-
 #ifndef __HAVE_ARCH_MEMSET16
 /**
  * memset16() - Fill a memory area with a uint16_t
-- 
2.21.0

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

[Hans de Goede]

Hi,

On 07-10-2019 20:42, Arvind Sankar wrote:
> On Mon, Oct 07, 2019 at 05:40:07PM +0200, Ingo Molnar wrote:
>>
>> * Arvind Sankar <nivedita@alum.mit.edu> wrote:
>>
>>> With the barrier in there, is there any reason to *not* inline the
>>> function? barrier_data() is an asm statement that tells the compiler
>>> that the asm uses the memory that was set to zero, thus preventing it
>>> from removing the memset even if nothing else uses that memory later. A
>>> more detailed comment is there in compiler-gcc.h. I can't see why it
>>> wouldn't work even if it were inlined.
>>>
>>> If the function can indeed be inlined, we could just make the common
>>> implementation a macro and avoid duplicating it? As mentioned in another
>>> mail, we otherwise will likely need another duplicate implementation for
>>> arch/s390/purgatory as well.
>>
>> I suspect macro would be justified in this case. Mind sending a v3 patch
>> to demonstrate how it would all look like?
>>
>> I'll zap v2 if the macro solution looks better.
>>
>> Thanks,
>>
>> 	Ingo
> 
> Patch attached to turn memzero_explicit into inline function.

Hehe, I had prepared and have just tested the exact same patch
(only the commit msg was different).

I've just booted a kernel build with that patch and that works
fine (as expected).

So your patch is:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>

Since this is a bit of a core change though, I think it is
best if you send it to the linux-kernel list (with my tags from above
added) as is normally done for kernel patches. Then others, who may
not be following this thread, will get a chance to give feedback on it.

Regards,

Hans

[PATCH] lib/string: make memzero_explicit inline instead of external

[Arvind Sankar]

With the use of the barrier implied by barrier_data(), there is no need
for memzero_explicit to be extern. Making it inline saves the overhead
of a function call, and allows the code to be reused in arch/*/purgatory
without having to duplicate the implementation.

Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 include/linux/string.h | 21 ++++++++++++++++++++-
 lib/string.c           | 21 ---------------------
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index b2f9df7f0761..b6ccdc2c7f02 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -227,7 +227,26 @@ static inline bool strstarts(const char *str, const char *prefix)
 }
 
 size_t memweight(const void *ptr, size_t bytes);
-void memzero_explicit(void *s, size_t count);
+
+/**
+ * memzero_explicit - Fill a region of memory (e.g. sensitive
+ *		      keying data) with 0s.
+ * @s: Pointer to the start of the area.
+ * @count: The size of the area.
+ *
+ * Note: usually using memset() is just fine (!), but in cases
+ * where clearing out _local_ data at the end of a scope is
+ * necessary, memzero_explicit() should be used instead in
+ * order to prevent the compiler from optimising away zeroing.
+ *
+ * memzero_explicit() doesn't need an arch-specific version as
+ * it just invokes the one of memset() implicitly.
+ */
+static inline void memzero_explicit(void *s, size_t count)
+{
+	memset(s, 0, count);
+	barrier_data(s);
+}
 
 /**
  * kbasename - return the last part of a pathname.
diff --git a/lib/string.c b/lib/string.c
index cd7a10c19210..08ec58cc673b 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -748,27 +748,6 @@ void *memset(void *s, int c, size_t count)
 EXPORT_SYMBOL(memset);
 #endif
 
-/**
- * memzero_explicit - Fill a region of memory (e.g. sensitive
- *		      keying data) with 0s.
- * @s: Pointer to the start of the area.
- * @count: The size of the area.
- *
- * Note: usually using memset() is just fine (!), but in cases
- * where clearing out _local_ data at the end of a scope is
- * necessary, memzero_explicit() should be used instead in
- * order to prevent the compiler from optimising away zeroing.
- *
- * memzero_explicit() doesn't need an arch-specific version as
- * it just invokes the one of memset() implicitly.
- */
-void memzero_explicit(void *s, size_t count)
-{
-	memset(s, 0, count);
-	barrier_data(s);
-}
-EXPORT_SYMBOL(memzero_explicit);
-
 #ifndef __HAVE_ARCH_MEMSET16
 /**
  * memset16() - Fill a memory area with a uint16_t
-- 
2.21.0

[tip: x86/urgent] lib/string: Make memzero_explicit() inline instead of external

[tip-bot2 for Arvind Sankar]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     bec500777089b3c96c53681fc0aa6fee59711d4a
Gitweb:        https://git.kernel.org/tip/bec500777089b3c96c53681fc0aa6fee59711d4a
Author:        Arvind Sankar <nivedita@alum.mit.edu>
AuthorDate:    Mon, 07 Oct 2019 18:00:02 -04:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 08 Oct 2019 13:27:05 +02:00

lib/string: Make memzero_explicit() inline instead of external

With the use of the barrier implied by barrier_data(), there is no need
for memzero_explicit() to be extern. Making it inline saves the overhead
of a function call, and allows the code to be reused in arch/*/purgatory
without having to duplicate the implementation.

Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H . Peter Anvin <hpa@zytor.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephan Mueller <smueller@chronox.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-s390@vger.kernel.org
Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
Link: https://lkml.kernel.org/r/20191007220000.GA408752@rani.riverdale.lan
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/string.h | 21 ++++++++++++++++++++-
 lib/string.c           | 21 ---------------------
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index b2f9df7..b6ccdc2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -227,7 +227,26 @@ static inline bool strstarts(const char *str, const char *prefix)
 }
 
 size_t memweight(const void *ptr, size_t bytes);
-void memzero_explicit(void *s, size_t count);
+
+/**
+ * memzero_explicit - Fill a region of memory (e.g. sensitive
+ *		      keying data) with 0s.
+ * @s: Pointer to the start of the area.
+ * @count: The size of the area.
+ *
+ * Note: usually using memset() is just fine (!), but in cases
+ * where clearing out _local_ data at the end of a scope is
+ * necessary, memzero_explicit() should be used instead in
+ * order to prevent the compiler from optimising away zeroing.
+ *
+ * memzero_explicit() doesn't need an arch-specific version as
+ * it just invokes the one of memset() implicitly.
+ */
+static inline void memzero_explicit(void *s, size_t count)
+{
+	memset(s, 0, count);
+	barrier_data(s);
+}
 
 /**
  * kbasename - return the last part of a pathname.
diff --git a/lib/string.c b/lib/string.c
index cd7a10c..08ec58c 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -748,27 +748,6 @@ void *memset(void *s, int c, size_t count)
 EXPORT_SYMBOL(memset);
 #endif
 
-/**
- * memzero_explicit - Fill a region of memory (e.g. sensitive
- *		      keying data) with 0s.
- * @s: Pointer to the start of the area.
- * @count: The size of the area.
- *
- * Note: usually using memset() is just fine (!), but in cases
- * where clearing out _local_ data at the end of a scope is
- * necessary, memzero_explicit() should be used instead in
- * order to prevent the compiler from optimising away zeroing.
- *
- * memzero_explicit() doesn't need an arch-specific version as
- * it just invokes the one of memset() implicitly.
- */
-void memzero_explicit(void *s, size_t count)
-{
-	memset(s, 0, count);
-	barrier_data(s);
-}
-EXPORT_SYMBOL(memzero_explicit);
-
 #ifndef __HAVE_ARCH_MEMSET16
 /**
  * memset16() - Fill a memory area with a uint16_t

[tip: x86/urgent] lib/string: Make memzero_explicit() inline instead of external

[tip-bot2 for Arvind Sankar]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     bec500777089b3c96c53681fc0aa6fee59711d4a
Gitweb:        https://git.kernel.org/tip/bec500777089b3c96c53681fc0aa6fee59711d4a
Author:        Arvind Sankar <nivedita@alum.mit.edu>
AuthorDate:    Mon, 07 Oct 2019 18:00:02 -04:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Tue, 08 Oct 2019 13:27:05 +02:00

lib/string: Make memzero_explicit() inline instead of external

With the use of the barrier implied by barrier_data(), there is no need
for memzero_explicit() to be extern. Making it inline saves the overhead
of a function call, and allows the code to be reused in arch/*/purgatory
without having to duplicate the implementation.

Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H . Peter Anvin <hpa@zytor.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephan Mueller <smueller@chronox.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Cc: linux-s390@vger.kernel.org
Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
Link: https://lkml.kernel.org/r/20191007220000.GA408752@rani.riverdale.lan
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 include/linux/string.h | 21 ++++++++++++++++++++-
 lib/string.c           | 21 ---------------------
 2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/include/linux/string.h b/include/linux/string.h
index b2f9df7..b6ccdc2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -227,7 +227,26 @@ static inline bool strstarts(const char *str, const char *prefix)
 }
 
 size_t memweight(const void *ptr, size_t bytes);
-void memzero_explicit(void *s, size_t count);
+
+/**
+ * memzero_explicit - Fill a region of memory (e.g. sensitive
+ *		      keying data) with 0s.
+ * @s: Pointer to the start of the area.
+ * @count: The size of the area.
+ *
+ * Note: usually using memset() is just fine (!), but in cases
+ * where clearing out _local_ data at the end of a scope is
+ * necessary, memzero_explicit() should be used instead in
+ * order to prevent the compiler from optimising away zeroing.
+ *
+ * memzero_explicit() doesn't need an arch-specific version as
+ * it just invokes the one of memset() implicitly.
+ */
+static inline void memzero_explicit(void *s, size_t count)
+{
+	memset(s, 0, count);
+	barrier_data(s);
+}
 
 /**
  * kbasename - return the last part of a pathname.
diff --git a/lib/string.c b/lib/string.c
index cd7a10c..08ec58c 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -748,27 +748,6 @@ void *memset(void *s, int c, size_t count)
 EXPORT_SYMBOL(memset);
 #endif
 
-/**
- * memzero_explicit - Fill a region of memory (e.g. sensitive
- *		      keying data) with 0s.
- * @s: Pointer to the start of the area.
- * @count: The size of the area.
- *
- * Note: usually using memset() is just fine (!), but in cases
- * where clearing out _local_ data at the end of a scope is
- * necessary, memzero_explicit() should be used instead in
- * order to prevent the compiler from optimising away zeroing.
- *
- * memzero_explicit() doesn't need an arch-specific version as
- * it just invokes the one of memset() implicitly.
- */
-void memzero_explicit(void *s, size_t count)
-{
-	memset(s, 0, count);
-	barrier_data(s);
-}
-EXPORT_SYMBOL(memzero_explicit);
-
 #ifndef __HAVE_ARCH_MEMSET16
 /**
  * memset16() - Fill a memory area with a uint16_t

Re: [PATCH] lib/string: make memzero_explicit inline instead of external

[Dave Young]

On 10/07/19 at 06:00pm, Arvind Sankar wrote:
> With the use of the barrier implied by barrier_data(), there is no need
> for memzero_explicit to be extern. Making it inline saves the overhead
> of a function call, and allows the code to be reused in arch/*/purgatory
> without having to duplicate the implementation.
> 
> Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  include/linux/string.h | 21 ++++++++++++++++++++-
>  lib/string.c           | 21 ---------------------
>  2 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b2f9df7f0761..b6ccdc2c7f02 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -227,7 +227,26 @@ static inline bool strstarts(const char *str, const char *prefix)
>  }
>  
>  size_t memweight(const void *ptr, size_t bytes);
> -void memzero_explicit(void *s, size_t count);
> +
> +/**
> + * memzero_explicit - Fill a region of memory (e.g. sensitive
> + *		      keying data) with 0s.
> + * @s: Pointer to the start of the area.
> + * @count: The size of the area.
> + *
> + * Note: usually using memset() is just fine (!), but in cases
> + * where clearing out _local_ data at the end of a scope is
> + * necessary, memzero_explicit() should be used instead in
> + * order to prevent the compiler from optimising away zeroing.
> + *
> + * memzero_explicit() doesn't need an arch-specific version as
> + * it just invokes the one of memset() implicitly.
> + */
> +static inline void memzero_explicit(void *s, size_t count)
> +{
> +	memset(s, 0, count);
> +	barrier_data(s);
> +}
>  
>  /**
>   * kbasename - return the last part of a pathname.
> diff --git a/lib/string.c b/lib/string.c
> index cd7a10c19210..08ec58cc673b 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -748,27 +748,6 @@ void *memset(void *s, int c, size_t count)
>  EXPORT_SYMBOL(memset);
>  #endif
>  
> -/**
> - * memzero_explicit - Fill a region of memory (e.g. sensitive
> - *		      keying data) with 0s.
> - * @s: Pointer to the start of the area.
> - * @count: The size of the area.
> - *
> - * Note: usually using memset() is just fine (!), but in cases
> - * where clearing out _local_ data at the end of a scope is
> - * necessary, memzero_explicit() should be used instead in
> - * order to prevent the compiler from optimising away zeroing.
> - *
> - * memzero_explicit() doesn't need an arch-specific version as
> - * it just invokes the one of memset() implicitly.
> - */
> -void memzero_explicit(void *s, size_t count)
> -{
> -	memset(s, 0, count);
> -	barrier_data(s);
> -}
> -EXPORT_SYMBOL(memzero_explicit);
> -
>  #ifndef __HAVE_ARCH_MEMSET16
>  /**
>   * memset16() - Fill a memory area with a uint16_t
> -- 

Thanks for the fix!  Ccing kexec list since the problem is kexec/kdump
related.  People can try it when they see same issue.

Dave

Re: [PATCH] lib/string: make memzero_explicit inline instead of external

[Dave Young]

On 10/07/19 at 06:00pm, Arvind Sankar wrote:
> With the use of the barrier implied by barrier_data(), there is no need
> for memzero_explicit to be extern. Making it inline saves the overhead
> of a function call, and allows the code to be reused in arch/*/purgatory
> without having to duplicate the implementation.
> 
> Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> ---
>  include/linux/string.h | 21 ++++++++++++++++++++-
>  lib/string.c           | 21 ---------------------
>  2 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index b2f9df7f0761..b6ccdc2c7f02 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -227,7 +227,26 @@ static inline bool strstarts(const char *str, const char *prefix)
>  }
>  
>  size_t memweight(const void *ptr, size_t bytes);
> -void memzero_explicit(void *s, size_t count);
> +
> +/**
> + * memzero_explicit - Fill a region of memory (e.g. sensitive
> + *		      keying data) with 0s.
> + * @s: Pointer to the start of the area.
> + * @count: The size of the area.
> + *
> + * Note: usually using memset() is just fine (!), but in cases
> + * where clearing out _local_ data at the end of a scope is
> + * necessary, memzero_explicit() should be used instead in
> + * order to prevent the compiler from optimising away zeroing.
> + *
> + * memzero_explicit() doesn't need an arch-specific version as
> + * it just invokes the one of memset() implicitly.
> + */
> +static inline void memzero_explicit(void *s, size_t count)
> +{
> +	memset(s, 0, count);
> +	barrier_data(s);
> +}
>  
>  /**
>   * kbasename - return the last part of a pathname.
> diff --git a/lib/string.c b/lib/string.c
> index cd7a10c19210..08ec58cc673b 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -748,27 +748,6 @@ void *memset(void *s, int c, size_t count)
>  EXPORT_SYMBOL(memset);
>  #endif
>  
> -/**
> - * memzero_explicit - Fill a region of memory (e.g. sensitive
> - *		      keying data) with 0s.
> - * @s: Pointer to the start of the area.
> - * @count: The size of the area.
> - *
> - * Note: usually using memset() is just fine (!), but in cases
> - * where clearing out _local_ data at the end of a scope is
> - * necessary, memzero_explicit() should be used instead in
> - * order to prevent the compiler from optimising away zeroing.
> - *
> - * memzero_explicit() doesn't need an arch-specific version as
> - * it just invokes the one of memset() implicitly.
> - */
> -void memzero_explicit(void *s, size_t count)
> -{
> -	memset(s, 0, count);
> -	barrier_data(s);
> -}
> -EXPORT_SYMBOL(memzero_explicit);
> -
>  #ifndef __HAVE_ARCH_MEMSET16
>  /**
>   * memset16() - Fill a memory area with a uint16_t
> -- 

Thanks for the fix!  Ccing kexec list since the problem is kexec/kdump
related.  People can try it when they see same issue.

Dave

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] lib/string: make memzero_explicit inline instead of external

[Dave Young]

On 10/10/19 at 10:52am, Dave Young wrote:
> On 10/07/19 at 06:00pm, Arvind Sankar wrote:
> > With the use of the barrier implied by barrier_data(), there is no need
> > for memzero_explicit to be extern. Making it inline saves the overhead
> > of a function call, and allows the code to be reused in arch/*/purgatory
> > without having to duplicate the implementation.
> > 
> > Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > ---
> >  include/linux/string.h | 21 ++++++++++++++++++++-
> >  lib/string.c           | 21 ---------------------
> >  2 files changed, 20 insertions(+), 22 deletions(-)
> > 
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index b2f9df7f0761..b6ccdc2c7f02 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -227,7 +227,26 @@ static inline bool strstarts(const char *str, const char *prefix)
> >  }
> >  
> >  size_t memweight(const void *ptr, size_t bytes);
> > -void memzero_explicit(void *s, size_t count);
> > +
> > +/**
> > + * memzero_explicit - Fill a region of memory (e.g. sensitive
> > + *		      keying data) with 0s.
> > + * @s: Pointer to the start of the area.
> > + * @count: The size of the area.
> > + *
> > + * Note: usually using memset() is just fine (!), but in cases
> > + * where clearing out _local_ data at the end of a scope is
> > + * necessary, memzero_explicit() should be used instead in
> > + * order to prevent the compiler from optimising away zeroing.
> > + *
> > + * memzero_explicit() doesn't need an arch-specific version as
> > + * it just invokes the one of memset() implicitly.
> > + */
> > +static inline void memzero_explicit(void *s, size_t count)
> > +{
> > +	memset(s, 0, count);
> > +	barrier_data(s);
> > +}
> >  
> >  /**
> >   * kbasename - return the last part of a pathname.
> > diff --git a/lib/string.c b/lib/string.c
> > index cd7a10c19210..08ec58cc673b 100644
> > --- a/lib/string.c
> > +++ b/lib/string.c
> > @@ -748,27 +748,6 @@ void *memset(void *s, int c, size_t count)
> >  EXPORT_SYMBOL(memset);
> >  #endif
> >  
> > -/**
> > - * memzero_explicit - Fill a region of memory (e.g. sensitive
> > - *		      keying data) with 0s.
> > - * @s: Pointer to the start of the area.
> > - * @count: The size of the area.
> > - *
> > - * Note: usually using memset() is just fine (!), but in cases
> > - * where clearing out _local_ data at the end of a scope is
> > - * necessary, memzero_explicit() should be used instead in
> > - * order to prevent the compiler from optimising away zeroing.
> > - *
> > - * memzero_explicit() doesn't need an arch-specific version as
> > - * it just invokes the one of memset() implicitly.
> > - */
> > -void memzero_explicit(void *s, size_t count)
> > -{
> > -	memset(s, 0, count);
> > -	barrier_data(s);
> > -}
> > -EXPORT_SYMBOL(memzero_explicit);
> > -
> >  #ifndef __HAVE_ARCH_MEMSET16
> >  /**
> >   * memset16() - Fill a memory area with a uint16_t
> > -- 
> 
> Thanks for the fix!  Ccing kexec list since the problem is kexec/kdump
> related.  People can try it when they see same issue.
> 

Also:

Tested-by: Dave Young <dyoung@redhat.com>

Thanks
Dave

Re: [PATCH] lib/string: make memzero_explicit inline instead of external

[Dave Young]

On 10/10/19 at 10:52am, Dave Young wrote:
> On 10/07/19 at 06:00pm, Arvind Sankar wrote:
> > With the use of the barrier implied by barrier_data(), there is no need
> > for memzero_explicit to be extern. Making it inline saves the overhead
> > of a function call, and allows the code to be reused in arch/*/purgatory
> > without having to duplicate the implementation.
> > 
> > Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> > ---
> >  include/linux/string.h | 21 ++++++++++++++++++++-
> >  lib/string.c           | 21 ---------------------
> >  2 files changed, 20 insertions(+), 22 deletions(-)
> > 
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index b2f9df7f0761..b6ccdc2c7f02 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -227,7 +227,26 @@ static inline bool strstarts(const char *str, const char *prefix)
> >  }
> >  
> >  size_t memweight(const void *ptr, size_t bytes);
> > -void memzero_explicit(void *s, size_t count);
> > +
> > +/**
> > + * memzero_explicit - Fill a region of memory (e.g. sensitive
> > + *		      keying data) with 0s.
> > + * @s: Pointer to the start of the area.
> > + * @count: The size of the area.
> > + *
> > + * Note: usually using memset() is just fine (!), but in cases
> > + * where clearing out _local_ data at the end of a scope is
> > + * necessary, memzero_explicit() should be used instead in
> > + * order to prevent the compiler from optimising away zeroing.
> > + *
> > + * memzero_explicit() doesn't need an arch-specific version as
> > + * it just invokes the one of memset() implicitly.
> > + */
> > +static inline void memzero_explicit(void *s, size_t count)
> > +{
> > +	memset(s, 0, count);
> > +	barrier_data(s);
> > +}
> >  
> >  /**
> >   * kbasename - return the last part of a pathname.
> > diff --git a/lib/string.c b/lib/string.c
> > index cd7a10c19210..08ec58cc673b 100644
> > --- a/lib/string.c
> > +++ b/lib/string.c
> > @@ -748,27 +748,6 @@ void *memset(void *s, int c, size_t count)
> >  EXPORT_SYMBOL(memset);
> >  #endif
> >  
> > -/**
> > - * memzero_explicit - Fill a region of memory (e.g. sensitive
> > - *		      keying data) with 0s.
> > - * @s: Pointer to the start of the area.
> > - * @count: The size of the area.
> > - *
> > - * Note: usually using memset() is just fine (!), but in cases
> > - * where clearing out _local_ data at the end of a scope is
> > - * necessary, memzero_explicit() should be used instead in
> > - * order to prevent the compiler from optimising away zeroing.
> > - *
> > - * memzero_explicit() doesn't need an arch-specific version as
> > - * it just invokes the one of memset() implicitly.
> > - */
> > -void memzero_explicit(void *s, size_t count)
> > -{
> > -	memset(s, 0, count);
> > -	barrier_data(s);
> > -}
> > -EXPORT_SYMBOL(memzero_explicit);
> > -
> >  #ifndef __HAVE_ARCH_MEMSET16
> >  /**
> >   * memset16() - Fill a memory area with a uint16_t
> > -- 
> 
> Thanks for the fix!  Ccing kexec list since the problem is kexec/kdump
> related.  People can try it when they see same issue.
> 

Also:

Tested-by: Dave Young <dyoung@redhat.com>

Thanks
Dave


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[tip: x86/urgent] x86/boot: Provide memzero_explicit()

[tip-bot2 for Hans de Goede]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     ee008a19f1c72c37ffa54326a592035dddb66fd6
Gitweb:        https://git.kernel.org/tip/ee008a19f1c72c37ffa54326a592035dddb66fd6
Author:        Hans de Goede <hdegoede@redhat.com>
AuthorDate:    Mon, 07 Oct 2019 15:47:24 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 07 Oct 2019 16:47:35 +02:00

x86/boot: Provide memzero_explicit()

The purgatory code now uses the shared lib/crypto/sha256.c sha256
implementation. This needs memzero_explicit(), implement this.

We also have barrier_data() call after the memset, making sure
neither the compiler nor the linker optimizes out this seemingly
unused function.

Reported-by: Arvind Sankar <nivedita@alum.mit.edu>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H . Peter Anvin <hpa@zytor.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
Link: https://lkml.kernel.org/r/20191007134724.4019-1-hdegoede@redhat.com
[ Added comment. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/boot/compressed/string.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 81fc1ea..dd30e63 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -50,6 +50,16 @@ void *memset(void *s, int c, size_t n)
 	return s;
 }
 
+void memzero_explicit(void *s, size_t count)
+{
+	memset(s, 0, count);
+	/*
+	 * Make sure this function never gets inlined and
+	 * the memset() never gets optimized away:
+	 */
+	barrier_data(s);
+}
+
 void *memmove(void *dest, const void *src, size_t n)
 {
 	unsigned char *d = dest;

[tip: x86/urgent] x86/boot: Provide memzero_explicit()

[tip-bot2 for Hans de Goede]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     ee008a19f1c72c37ffa54326a592035dddb66fd6
Gitweb:        https://git.kernel.org/tip/ee008a19f1c72c37ffa54326a592035dddb66fd6
Author:        Hans de Goede <hdegoede@redhat.com>
AuthorDate:    Mon, 07 Oct 2019 15:47:24 +02:00
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Mon, 07 Oct 2019 16:47:35 +02:00

x86/boot: Provide memzero_explicit()

The purgatory code now uses the shared lib/crypto/sha256.c sha256
implementation. This needs memzero_explicit(), implement this.

We also have barrier_data() call after the memset, making sure
neither the compiler nor the linker optimizes out this seemingly
unused function.

Reported-by: Arvind Sankar <nivedita@alum.mit.edu>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: H . Peter Anvin <hpa@zytor.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-crypto@vger.kernel.org
Fixes: 906a4bb97f5d ("crypto: sha256 - Use get/put_unaligned_be32 to get input, memzero_explicit")
Link: https://lkml.kernel.org/r/20191007134724.4019-1-hdegoede@redhat.com
[ Added comment. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/boot/compressed/string.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 81fc1ea..dd30e63 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -50,6 +50,16 @@ void *memset(void *s, int c, size_t n)
 	return s;
 }
 
+void memzero_explicit(void *s, size_t count)
+{
+	memset(s, 0, count);
+	/*
+	 * Make sure this function never gets inlined and
+	 * the memset() never gets optimized away:
+	 */
+	barrier_data(s);
+}
+
 void *memmove(void *dest, const void *src, size_t n)
 {
 	unsigned char *d = dest;