* [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit @ 2019-10-07 13:47 Hans de Goede 2019-10-07 14:00 ` Ingo Molnar ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Hans de Goede @ 2019-10-07 13:47 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin Cc: Hans de Goede, Herbert Xu, Ard Biesheuvel, linux-crypto, x86, linux-kernel, Arvind Sankar 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit 2019-10-07 13:47 [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit Hans de Goede @ 2019-10-07 14:00 ` Ingo Molnar 2019-10-07 14:11 ` Hans de Goede 2019-10-07 14:49 ` [tip: x86/urgent] x86/boot: Provide memzero_explicit() tip-bot2 for Hans de Goede 2019-10-07 14:49 ` tip-bot2 for Hans de Goede 2 siblings, 1 reply; 17+ messages in thread From: Ingo Molnar @ 2019-10-07 14:00 UTC (permalink / raw) To: Hans de Goede Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, linux-crypto, x86, linux-kernel, Arvind Sankar * 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit 2019-10-07 14:00 ` Ingo Molnar @ 2019-10-07 14:11 ` Hans de Goede 2019-10-07 14:22 ` Ingo Molnar 0 siblings, 1 reply; 17+ messages in thread From: Hans de Goede @ 2019-10-07 14:11 UTC (permalink / raw) To: Ingo Molnar Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, linux-crypto, x86, linux-kernel, Arvind Sankar, Stephan Mueller 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit 2019-10-07 14:11 ` Hans de Goede @ 2019-10-07 14:22 ` Ingo Molnar 2019-10-07 14:29 ` Hans de Goede 0 siblings, 1 reply; 17+ messages in thread From: Ingo Molnar @ 2019-10-07 14:22 UTC (permalink / raw) To: Hans de Goede Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, linux-crypto, x86, linux-kernel, Arvind Sankar, Stephan Mueller * 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit 2019-10-07 14:22 ` Ingo Molnar @ 2019-10-07 14:29 ` Hans de Goede 2019-10-07 14:46 ` Ingo Molnar 0 siblings, 1 reply; 17+ messages in thread From: Hans de Goede @ 2019-10-07 14:29 UTC (permalink / raw) To: Ingo Molnar Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, linux-crypto, x86, linux-kernel, Arvind Sankar, Stephan Mueller 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit 2019-10-07 14:29 ` Hans de Goede @ 2019-10-07 14:46 ` Ingo Molnar 2019-10-07 15:20 ` Arvind Sankar 0 siblings, 1 reply; 17+ messages in thread From: Ingo Molnar @ 2019-10-07 14:46 UTC (permalink / raw) To: Hans de Goede Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, linux-crypto, x86, linux-kernel, Arvind Sankar, Stephan Mueller * 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit 2019-10-07 14:46 ` Ingo Molnar @ 2019-10-07 15:20 ` Arvind Sankar 2019-10-07 15:40 ` Ingo Molnar 0 siblings, 1 reply; 17+ messages in thread From: Arvind Sankar @ 2019-10-07 15:20 UTC (permalink / raw) To: Ingo Molnar Cc: Hans de Goede, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, linux-crypto, x86, linux-kernel, Arvind Sankar, Stephan Mueller 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit 2019-10-07 15:20 ` Arvind Sankar @ 2019-10-07 15:40 ` Ingo Molnar 2019-10-07 18:42 ` Arvind Sankar 0 siblings, 1 reply; 17+ messages in thread From: Ingo Molnar @ 2019-10-07 15:40 UTC (permalink / raw) To: Arvind Sankar Cc: Hans de Goede, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, linux-crypto, x86, linux-kernel, Stephan Mueller * 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit 2019-10-07 15:40 ` Ingo Molnar @ 2019-10-07 18:42 ` Arvind Sankar 2019-10-07 19:36 ` Hans de Goede 0 siblings, 1 reply; 17+ messages in thread From: Arvind Sankar @ 2019-10-07 18:42 UTC (permalink / raw) To: Ingo Molnar Cc: Arvind Sankar, Hans de Goede, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, linux-crypto, x86, linux-kernel, Stephan Mueller, linux-s390 [-- 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit 2019-10-07 18:42 ` Arvind Sankar @ 2019-10-07 19:36 ` Hans de Goede 2019-10-07 22:00 ` [PATCH] lib/string: make memzero_explicit inline instead of external Arvind Sankar 0 siblings, 1 reply; 17+ messages in thread From: Hans de Goede @ 2019-10-07 19:36 UTC (permalink / raw) To: Arvind Sankar, Ingo Molnar Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, linux-crypto, x86, linux-kernel, Stephan Mueller, linux-s390 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] lib/string: make memzero_explicit inline instead of external 2019-10-07 19:36 ` Hans de Goede @ 2019-10-07 22:00 ` Arvind Sankar 2019-10-08 11:33 ` [tip: x86/urgent] lib/string: Make memzero_explicit() " tip-bot2 for Arvind Sankar ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Arvind Sankar @ 2019-10-07 22:00 UTC (permalink / raw) To: linux-kernel, Ingo Molnar, Hans de Goede Cc: linux-crypto, linux-s390, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, Stephan Mueller 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [tip: x86/urgent] lib/string: Make memzero_explicit() inline instead of external 2019-10-07 22:00 ` [PATCH] lib/string: make memzero_explicit inline instead of external Arvind Sankar @ 2019-10-08 11:33 ` tip-bot2 for Arvind Sankar 2019-10-08 11:33 ` tip-bot2 for Arvind Sankar 2019-10-10 2:52 ` [PATCH] lib/string: make memzero_explicit " Dave Young 2 siblings, 0 replies; 17+ messages in thread From: tip-bot2 for Arvind Sankar @ 2019-10-08 11:33 UTC (permalink / raw) To: linux-tip-commits Cc: Hans de Goede, Arvind Sankar, Ard Biesheuvel, Borislav Petkov, H . Peter Anvin, Herbert Xu, Linus Torvalds, Peter Zijlstra, Stephan Mueller, Thomas Gleixner, linux-crypto, linux-s390, Ingo Molnar, linux-kernel 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [tip: x86/urgent] lib/string: Make memzero_explicit() inline instead of external 2019-10-07 22:00 ` [PATCH] lib/string: make memzero_explicit inline instead of external Arvind Sankar 2019-10-08 11:33 ` [tip: x86/urgent] lib/string: Make memzero_explicit() " tip-bot2 for Arvind Sankar @ 2019-10-08 11:33 ` tip-bot2 for Arvind Sankar 2019-10-10 2:52 ` [PATCH] lib/string: make memzero_explicit " Dave Young 2 siblings, 0 replies; 17+ messages in thread From: tip-bot2 for Arvind Sankar @ 2019-10-08 11:33 UTC (permalink / raw) To: linux-tip-commits Cc: Hans de Goede, Arvind Sankar, Ard Biesheuvel, Borislav Petkov, H . Peter Anvin, Herbert Xu, Linus Torvalds, Peter Zijlstra, Stephan Mueller, Thomas Gleixner, linux-crypto, linux-s390, Ingo Molnar, linux-kernel 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] lib/string: make memzero_explicit inline instead of external 2019-10-07 22:00 ` [PATCH] lib/string: make memzero_explicit inline instead of external Arvind Sankar 2019-10-08 11:33 ` [tip: x86/urgent] lib/string: Make memzero_explicit() " tip-bot2 for Arvind Sankar 2019-10-08 11:33 ` tip-bot2 for Arvind Sankar @ 2019-10-10 2:52 ` Dave Young 2019-10-10 6:56 ` Dave Young 2 siblings, 1 reply; 17+ messages in thread From: Dave Young @ 2019-10-10 2:52 UTC (permalink / raw) To: Arvind Sankar Cc: linux-kernel, Ingo Molnar, Hans de Goede, linux-crypto, linux-s390, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, Stephan Mueller, kexec 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] lib/string: make memzero_explicit inline instead of external 2019-10-10 2:52 ` [PATCH] lib/string: make memzero_explicit " Dave Young @ 2019-10-10 6:56 ` Dave Young 0 siblings, 0 replies; 17+ messages in thread From: Dave Young @ 2019-10-10 6:56 UTC (permalink / raw) To: Arvind Sankar Cc: linux-kernel, Ingo Molnar, Hans de Goede, linux-crypto, linux-s390, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin, Herbert Xu, Ard Biesheuvel, Stephan Mueller, kexec 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [tip: x86/urgent] x86/boot: Provide memzero_explicit() 2019-10-07 13:47 [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit Hans de Goede 2019-10-07 14:00 ` Ingo Molnar @ 2019-10-07 14:49 ` tip-bot2 for Hans de Goede 2019-10-07 14:49 ` tip-bot2 for Hans de Goede 2 siblings, 0 replies; 17+ messages in thread From: tip-bot2 for Hans de Goede @ 2019-10-07 14:49 UTC (permalink / raw) To: linux-tip-commits Cc: Arvind Sankar, Hans de Goede, Ard Biesheuvel, Borislav Petkov, H . Peter Anvin, Herbert Xu, Linus Torvalds, Peter Zijlstra, Thomas Gleixner, linux-crypto, Ingo Molnar, linux-kernel 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; ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [tip: x86/urgent] x86/boot: Provide memzero_explicit() 2019-10-07 13:47 [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit Hans de Goede 2019-10-07 14:00 ` Ingo Molnar 2019-10-07 14:49 ` [tip: x86/urgent] x86/boot: Provide memzero_explicit() tip-bot2 for Hans de Goede @ 2019-10-07 14:49 ` tip-bot2 for Hans de Goede 2 siblings, 0 replies; 17+ messages in thread From: tip-bot2 for Hans de Goede @ 2019-10-07 14:49 UTC (permalink / raw) To: linux-tip-commits Cc: Arvind Sankar, Hans de Goede, Ard Biesheuvel, Borislav Petkov, H . Peter Anvin, Herbert Xu, Linus Torvalds, Peter Zijlstra, Thomas Gleixner, linux-crypto, Ingo Molnar, linux-kernel 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; ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-10-10 6:56 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-07 13:47 [PATCH v2 5.4 regression fix] x86/boot: Provide memzero_explicit Hans de Goede 2019-10-07 14:00 ` Ingo Molnar 2019-10-07 14:11 ` Hans de Goede 2019-10-07 14:22 ` Ingo Molnar 2019-10-07 14:29 ` Hans de Goede 2019-10-07 14:46 ` Ingo Molnar 2019-10-07 15:20 ` Arvind Sankar 2019-10-07 15:40 ` Ingo Molnar 2019-10-07 18:42 ` Arvind Sankar 2019-10-07 19:36 ` Hans de Goede 2019-10-07 22:00 ` [PATCH] lib/string: make memzero_explicit inline instead of external Arvind Sankar 2019-10-08 11:33 ` [tip: x86/urgent] lib/string: Make memzero_explicit() " tip-bot2 for Arvind Sankar 2019-10-08 11:33 ` tip-bot2 for Arvind Sankar 2019-10-10 2:52 ` [PATCH] lib/string: make memzero_explicit " Dave Young 2019-10-10 6:56 ` Dave Young 2019-10-07 14:49 ` [tip: x86/urgent] x86/boot: Provide memzero_explicit() tip-bot2 for Hans de Goede 2019-10-07 14:49 ` tip-bot2 for Hans de Goede
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).