[PATCH] crypto: x86/aes-ni: fix AVX detection

[PATCH] crypto: x86/aes-ni: fix AVX detection

[Maxim Levitsky]

Fix two semi-theoretical issues that are present.

1. AVX is assumed to be present when AVX2 is present.
 That can be false in a VM.
 This can be considered a hypervisor bug,
 but the kernel should not crash in this case if this is possible.

2. YMM state can be soft disabled in XCR0.

Fix both issues by using 'cpu_has_xfeatures(XFEATURE_MASK_YMM')
to check for usable AVX support.

Fixes: d764593af9249 ("crypto: aesni - AVX and AVX2 version of AESNI-GCM encode and decode")

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/crypto/aesni-intel_glue.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
index 0fc961bef299c..20db1e500ef6f 100644
--- a/arch/x86/crypto/aesni-intel_glue.c
+++ b/arch/x86/crypto/aesni-intel_glue.c
@@ -1147,24 +1147,25 @@ static int __init aesni_init(void)
 	if (!x86_match_cpu(aesni_cpu_id))
 		return -ENODEV;
 #ifdef CONFIG_X86_64
-	if (boot_cpu_has(X86_FEATURE_AVX2)) {
-		pr_info("AVX2 version of gcm_enc/dec engaged.\n");
-		static_branch_enable(&gcm_use_avx);
-		static_branch_enable(&gcm_use_avx2);
-	} else
-	if (boot_cpu_has(X86_FEATURE_AVX)) {
-		pr_info("AVX version of gcm_enc/dec engaged.\n");
+	if (cpu_has_xfeatures(XFEATURE_MASK_YMM, NULL)) {
+
 		static_branch_enable(&gcm_use_avx);
-	} else {
-		pr_info("SSE version of gcm_enc/dec engaged.\n");
-	}
-	if (boot_cpu_has(X86_FEATURE_AVX)) {
+
+		if (boot_cpu_has(X86_FEATURE_AVX2)) {
+			static_branch_enable(&gcm_use_avx2);
+			pr_info("AVX2 version of gcm_enc/dec engaged.\n");
+		} else {
+			pr_info("AVX version of gcm_enc/dec engaged.\n");
+		}
+
 		/* optimize performance of ctr mode encryption transform */
 		static_call_update(aesni_ctr_enc_tfm, aesni_ctr_enc_avx_tfm);
 		pr_info("AES CTR mode by8 optimization enabled\n");
+
+	} else {
+		pr_info("SSE version of gcm_enc/dec engaged.\n");
 	}
 #endif
-
 	err = crypto_register_alg(&aesni_cipher_alg);
 	if (err)
 		return err;
-- 
2.26.3

Re: [PATCH] crypto: x86/aes-ni: fix AVX detection

[Maxim Levitsky]

On Wed, 2021-11-03 at 14:46 +0200, Maxim Levitsky wrote:
> Fix two semi-theoretical issues that are present.
> 
> 1. AVX is assumed to be present when AVX2 is present.
>  That can be false in a VM.
>  This can be considered a hypervisor bug,
>  but the kernel should not crash in this case if this is possible.
> 
> 2. YMM state can be soft disabled in XCR0.
> 
> Fix both issues by using 'cpu_has_xfeatures(XFEATURE_MASK_YMM')
> to check for usable AVX support.
> 
> Fixes: d764593af9249 ("crypto: aesni - AVX and AVX2 version of AESNI-GCM encode and decode")
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>

I forgot to mention that Paolo Bonzini helped me with this patch,
especially with the way to detect XCR0 bits.

Best regards,
	Maxim Levitsky

> ---
>  arch/x86/crypto/aesni-intel_glue.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/crypto/aesni-intel_glue.c b/arch/x86/crypto/aesni-intel_glue.c
> index 0fc961bef299c..20db1e500ef6f 100644
> --- a/arch/x86/crypto/aesni-intel_glue.c
> +++ b/arch/x86/crypto/aesni-intel_glue.c
> @@ -1147,24 +1147,25 @@ static int __init aesni_init(void)
>  	if (!x86_match_cpu(aesni_cpu_id))
>  		return -ENODEV;
>  #ifdef CONFIG_X86_64
> -	if (boot_cpu_has(X86_FEATURE_AVX2)) {
> -		pr_info("AVX2 version of gcm_enc/dec engaged.\n");
> -		static_branch_enable(&gcm_use_avx);
> -		static_branch_enable(&gcm_use_avx2);
> -	} else
> -	if (boot_cpu_has(X86_FEATURE_AVX)) {
> -		pr_info("AVX version of gcm_enc/dec engaged.\n");
> +	if (cpu_has_xfeatures(XFEATURE_MASK_YMM, NULL)) {
> +
>  		static_branch_enable(&gcm_use_avx);
> -	} else {
> -		pr_info("SSE version of gcm_enc/dec engaged.\n");
> -	}
> -	if (boot_cpu_has(X86_FEATURE_AVX)) {
> +
> +		if (boot_cpu_has(X86_FEATURE_AVX2)) {
> +			static_branch_enable(&gcm_use_avx2);
> +			pr_info("AVX2 version of gcm_enc/dec engaged.\n");
> +		} else {
> +			pr_info("AVX version of gcm_enc/dec engaged.\n");
> +		}
> +
>  		/* optimize performance of ctr mode encryption transform */
>  		static_call_update(aesni_ctr_enc_tfm, aesni_ctr_enc_avx_tfm);
>  		pr_info("AES CTR mode by8 optimization enabled\n");
> +
> +	} else {
> +		pr_info("SSE version of gcm_enc/dec engaged.\n");
>  	}
>  #endif
> -
>  	err = crypto_register_alg(&aesni_cipher_alg);
>  	if (err)
>  		return err;

Re: [PATCH] crypto: x86/aes-ni: fix AVX detection

[Dave Hansen]

On 11/3/21 5:46 AM, Maxim Levitsky wrote:
> Fix two semi-theoretical issues that are present.
> 
> 1. AVX is assumed to be present when AVX2 is present.
>  That can be false in a VM.
>  This can be considered a hypervisor bug,
>  but the kernel should not crash in this case if this is possible.

The kernel shouldn't crash in this case.  We've got a software
dependency which should disable AVX2 if AVX is off:

static const struct cpuid_dep cpuid_deps[] = {
...
        { X86_FEATURE_AVX2,                     X86_FEATURE_AVX,      },


> 2. YMM state can be soft disabled in XCR0.
> 
> Fix both issues by using 'cpu_has_xfeatures(XFEATURE_MASK_YMM')
> to check for usable AVX support.

There's another table to ensure that this doesn't happen:

> static unsigned short xsave_cpuid_features[] __initdata = {
>         [XFEATURE_FP]                           = X86_FEATURE_FPU,
>         [XFEATURE_SSE]                          = X86_FEATURE_XMM,
>         [XFEATURE_YMM]                          = X86_FEATURE_AVX,

So, if XFEATURE_YMM isn't supported, X86_FEATURE_AVX should be cleared.

But, that's all how it's _supposed_ to work.  It's quite possible we've
got bugs somewhere, so if you're hitting an issue in practice please let
us know.

If this did end up confusing you and Paulo, that's not great either.
Any patches that make these dependency tables easier to find or grok
would be appreciated too.

Re: [PATCH] crypto: x86/aes-ni: fix AVX detection

[Herbert Xu]

On Wed, Nov 03, 2021 at 07:43:43AM -0700, Dave Hansen wrote:
>
> The kernel shouldn't crash in this case.  We've got a software
> dependency which should disable AVX2 if AVX is off:

It's qemu, I thought it was a qemu bug but Paulo disagrees.

Cheers,
-- 
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.txt

Re: [PATCH] crypto: x86/aes-ni: fix AVX detection

[Paolo Bonzini]

On 11/3/21 15:52, Herbert Xu wrote:
> On Wed, Nov 03, 2021 at 07:43:43AM -0700, Dave Hansen wrote:
>> The kernel shouldn't crash in this case.  We've got a software
>> dependency which should disable AVX2 if AVX is off:
> It's qemu, I thought it was a qemu bug but Paulo disagrees.

I don't disagree that QEMU could do better in preventing nonsensical 
combinations; however, independent of that you shouldn't use AVX2 
without factoring XCR0 in the decision.  This is true no matter if 
userspace/kernel or even bare metal/virt.

That does not have to be done directly with cpu_has_xfeatures as in 
Maxim's patch: if boot_cpu_has(AVX) returns false after 
xsave_cpuid_features has filtered it out, or if boot_cpu_has(AVX2) 
returns false after cpuid_deps has filtered it out, that's fine.  I 
guess Maxim can look at the pointers that Dave provided and check if 
there's another bug somewhere in arch/x86/kernel.

Paolo

Re: [PATCH] crypto: x86/aes-ni: fix AVX detection

[Maxim Levitsky]

On Wed, 2021-11-03 at 07:43 -0700, Dave Hansen wrote:
> On 11/3/21 5:46 AM, Maxim Levitsky wrote:
> > Fix two semi-theoretical issues that are present.
> > 
> > 1. AVX is assumed to be present when AVX2 is present.
> >  That can be false in a VM.
> >  This can be considered a hypervisor bug,
> >  but the kernel should not crash in this case if this is possible.
> 
> The kernel shouldn't crash in this case.  We've got a software
> dependency which should disable AVX2 if AVX is off:
> 
> static const struct cpuid_dep cpuid_deps[] = {
> ...
>         { X86_FEATURE_AVX2,                     X86_FEATURE_AVX,      },



Looks like this table is only used when someone calls setup_clear_cpu_cap/clear_cpu_cap
which is used in all kind of places to disable CPU features that fails various conditions
(like VMX/SMX when MSR_IA32_FEAT_CTL suddently faults, or to disable RDRAND when
it fails built-in selfcheck and such, and it is used when user disables features on
kernel cmd line like the 'noxsave'

Also we do have the 'filter_cpuid_features' which disables some CPUID features,
which depend on whole CPUID leaves to be there.
This is the only precendent of the kernel coping with a bogus CPUID given by the
hypervisor, and it is even mentioned in a comment near this code.


filter_cpuid_features can be extended to filter known bogus CPUID depedencies,
like case when AVX2 supported and AVX not supported in CPUID.
If you agree, then it seems the best case to deal with this issue.

> 
> 
> > 2. YMM state can be soft disabled in XCR0.
> > 
> > Fix both issues by using 'cpu_has_xfeatures(XFEATURE_MASK_YMM')
> > to check for usable AVX support.
> 
> There's another table to ensure that this doesn't happen:
> 
> > static unsigned short xsave_cpuid_features[] __initdata = {
> >         [XFEATURE_FP]                           = X86_FEATURE_FPU,
> >         [XFEATURE_SSE]                          = X86_FEATURE_XMM,
> >         [XFEATURE_YMM]                          = X86_FEATURE_AVX,
> 
> So, if XFEATURE_YMM isn't supported, X86_FEATURE_AVX should be cleared.

I afraid that it is the other way around - this table makes sure that
we disable 'XFEATURE_YMM' in the xsave header when we (the kernel) uses
the xsave instruction, if the X86_FEATURE_AVX is not supported.

However, I haven't found a way to disable selected xfeatures, but only the
'noxsave/noxsaves/etc' which disable the whole feature in cpuid,
and that indeed does trigger the disablement via 'cpuid_deps' table.

> 
> But, that's all how it's _supposed_ to work.  It's quite possible we've
> got bugs somewhere, so if you're hitting an issue in practice please let
> us know.
> 
> If this did end up confusing you and Paulo, that's not great either.
> Any patches that make these dependency tables easier to find or grok
> would be appreciated too.
> 


Sorry for very late reply, I haven't gotten to work on this bug for a while,
Best regards,
	Maxim Levitsky

Re: [PATCH] crypto: x86/aes-ni: fix AVX detection

[Dave Hansen]

On 6/8/22 04:29, Maxim Levitsky wrote:
> filter_cpuid_features can be extended to filter known bogus CPUID depedencies,
> like case when AVX2 supported and AVX not supported in CPUID.
> If you agree, then it seems the best case to deal with this issue.

Yes, it should have been doing this all along.   Patches to fix that
would be very welcome.