On Fri, May 16, 2025 at 07:34:06PM +0800, Herbert Xu wrote:
>
> So what's happened is that previously if you call sha256_update
> from lib/crypto it would only use the generic C code to perform
> the operation.
>
> This has now been changed to automatically use SIMD instructions
> which obviously blew up in your case.
In the interim you can go back to the old ways and disable SIMD
for lib/crypto sha256 with this patch:
---8<---
Disable SIMD usage in lib/crypto sha256 as it is causing crashes.
Reported-by: Borislav Petkov <bp@alien8.de>
Fixes: 950e5c84118c ("crypto: sha256 - support arch-optimized lib and expose through shash")
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/include/crypto/internal/sha2.h b/include/crypto/internal/sha2.h
index b9bccd3ff57f..e1b0308c0539 100644
--- a/include/crypto/internal/sha2.h
+++ b/include/crypto/internal/sha2.h
@@ -32,7 +32,7 @@ static inline void sha256_choose_blocks(
if (!IS_ENABLED(CONFIG_CRYPTO_ARCH_HAVE_LIB_SHA256) || force_generic)
sha256_blocks_generic(state, data, nblocks);
else if (IS_ENABLED(CONFIG_CRYPTO_ARCH_HAVE_LIB_SHA256_SIMD) &&
- (force_simd || crypto_simd_usable()))
+ force_simd)
sha256_blocks_simd(state, data, nblocks);
else
sha256_blocks_arch(state, data, nblocks);
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtOn Fri, May 16, 2025 at 07:48:52PM +0800, Herbert Xu wrote:
> On Fri, May 16, 2025 at 07:34:06PM +0800, Herbert Xu wrote:
> >
> > So what's happened is that previously if you call sha256_update
> > from lib/crypto it would only use the generic C code to perform
> > the operation.
> >
> > This has now been changed to automatically use SIMD instructions
> > which obviously blew up in your case.
>
> In the interim you can go back to the old ways and disable SIMD
> for lib/crypto sha256 with this patch:
>
> ---8<---
> Disable SIMD usage in lib/crypto sha256 as it is causing crashes.
>
> Reported-by: Borislav Petkov <bp@alien8.de>
> Fixes: 950e5c84118c ("crypto: sha256 - support arch-optimized lib and expose through shash")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
That's silly. We should just fix x86's irq_fpu_usable() to return false before
the CPU is properly initialized. It already checks a per-cpu bool, so it
shouldn't be too hard to fit that in.
Using the generic SHA-256 code explicitly is also an option, but ideally the
regular functions would just work.
- Eric
On Fri, May 16, 2025 at 10:03:16AM -0700, Eric Biggers wrote:
> That's silly. We should just fix x86's irq_fpu_usable() to return false
> before the CPU is properly initialized. It already checks a per-cpu bool, so
> it shouldn't be too hard to fit that in.
Probably.
There's a fpu__init_cpu() call almost right after load_ucode_ap() which causes
this thing.
I'm not sure how much initialized stuff you need for SHA256 SIMD... perhaps
swap fpu__init_cpu() and load_ucode_ap() calls after proper code audit whether
that's ok.
Or add a "is the FPU initialized" check, as you propose, which is probably
easier.
As always, the x86 CPU init path is nasty and needs careful auditing.
> Using the generic SHA-256 code explicitly is also an option,
Or that.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Fri, May 16, 2025 at 08:13:22PM +0200, Borislav Petkov wrote:
> On Fri, May 16, 2025 at 10:03:16AM -0700, Eric Biggers wrote:
> > That's silly. We should just fix x86's irq_fpu_usable() to return false
> > before the CPU is properly initialized. It already checks a per-cpu bool, so
> > it shouldn't be too hard to fit that in.
>
> Probably.
>
> There's a fpu__init_cpu() call almost right after load_ucode_ap() which causes
> this thing.
>
> I'm not sure how much initialized stuff you need for SHA256 SIMD... perhaps
> swap fpu__init_cpu() and load_ucode_ap() calls after proper code audit whether
> that's ok.
>
> Or add a "is the FPU initialized" check, as you propose, which is probably
> easier.
>
> As always, the x86 CPU init path is nasty and needs careful auditing.
There are a few different ways in which __apply_microcode_amd() can get called:
__apply_microcode_amd
load_ucode_amd_bsp
load_ucode_bsp
x86_64_start_kernel
apply_microcode_amd
load_ucode_amd_ap
load_ucode_ap
start_secondary
microcode_ops::apply_microcode
load_secondary
__load_primary
reload_ucode_amd
reload_early_microcode
microcode_bsp_resume
mc_syscore_ops::resume
syscore_resume
__restore_processor_state
restore_processor_state
What would you say about going back to my earlier plan to make irq_fpu_usable()
return false when irqs_disabled(), like what arm64 does? (As I had in
https://lore.kernel.org/lkml/20250220051325.340691-2-ebiggers@kernel.org/).
I think that would handle all these cases, as well as others. We'd need to fix
__save_processor_state() to save the FPU state directly without pretending that
it's using kernel-mode FPU, but I don't know of any issues besides that. Then
we could also delete the irqs_disabled() checks that I added to
kernel_fpu_begin() and kernel_fpu_end().
- Eric
On Fri, May 16 2025 at 12:06, Eric Biggers wrote:
> What would you say about going back to my earlier plan to make irq_fpu_usable()
> return false when irqs_disabled(), like what arm64 does? (As I had in
> https://lore.kernel.org/lkml/20250220051325.340691-2-ebiggers@kernel.org/).
> + return !this_cpu_read(in_kernel_fpu) &&
> + !in_hardirq() && !irqs_disabled() && !in_nmi();
The !in_hardirq() is redundant because hard interrupt context runs with
interrupts disabled.
> I think that would handle all these cases, as well as others. We'd need to fix
> __save_processor_state() to save the FPU state directly without pretending that
> it's using kernel-mode FPU, but I don't know of any issues besides
> that.
Looks about right.
> Then we could also delete the irqs_disabled() checks that I added to
> kernel_fpu_begin() and kernel_fpu_end().
Yes. That conditional locking is horrible.
Thanks,
tglx
On Fri, May 16, 2025 at 07:48:52PM +0800, Herbert Xu wrote:
> On Fri, May 16, 2025 at 07:34:06PM +0800, Herbert Xu wrote:
> >
> > So what's happened is that previously if you call sha256_update
> > from lib/crypto it would only use the generic C code to perform
> > the operation.
> >
> > This has now been changed to automatically use SIMD instructions
> > which obviously blew up in your case.
>
> In the interim you can go back to the old ways and disable SIMD
> for lib/crypto sha256 with this patch:
>
> ---8<---
> Disable SIMD usage in lib/crypto sha256 as it is causing crashes.
>
> Reported-by: Borislav Petkov <bp@alien8.de>
Please make that
Reported-by: Ayush Jain <Ayush.Jain3@amd.com>
I'm just the messenger.
> Fixes: 950e5c84118c ("crypto: sha256 - support arch-optimized lib and expose through shash")
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
>
> diff --git a/include/crypto/internal/sha2.h b/include/crypto/internal/sha2.h
> index b9bccd3ff57f..e1b0308c0539 100644
> --- a/include/crypto/internal/sha2.h
> +++ b/include/crypto/internal/sha2.h
> @@ -32,7 +32,7 @@ static inline void sha256_choose_blocks(
> if (!IS_ENABLED(CONFIG_CRYPTO_ARCH_HAVE_LIB_SHA256) || force_generic)
> sha256_blocks_generic(state, data, nblocks);
> else if (IS_ENABLED(CONFIG_CRYPTO_ARCH_HAVE_LIB_SHA256_SIMD) &&
> - (force_simd || crypto_simd_usable()))
> + force_simd)
> sha256_blocks_simd(state, data, nblocks);
> else
> sha256_blocks_arch(state, data, nblocks);
> --
If you end up doing this, that fixes it, obviously:
Tested-by: Ayush Jain <Ayush.Jain3@amd.com>
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
© 2016 - 2025 Red Hat, Inc.