Re: [PATCH v2] crypto: x86/aegis,morus - Fix and simplify CPUID checks

2018-08-07 Thread Herbert Xu
On Fri, Aug 03, 2018 at 01:37:50PM +0200, Ondrej Mosnacek wrote:
> It turns out I had misunderstood how the x86_match_cpu() function works.
> It evaluates a logical OR of the matching conditions, not logical AND.
> This caused the CPU feature checks for AEGIS to pass even if only SSE2
> (but not AES-NI) was supported (or vice versa), leading to potential
> crashes if something tried to use the registered algs.
> 
> This patch switches the checks to a simpler method that is used e.g. in
> the Camellia x86 code.
> 
> The patch also removes the MODULE_DEVICE_TABLE declarations which
> actually seem to cause the modules to be auto-loaded at boot, which is
> not desired. The crypto API on-demand module loading is sufficient.
> 
> Fixes: 1d373d4e8e15 ("crypto: x86 - Add optimized AEGIS implementations")
> Fixes: 6ecc9d9ff91f ("crypto: x86 - Add optimized MORUS implementations")
> Signed-off-by: Ondrej Mosnacek 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt


Re: [PATCH v2] crypto: x86/aegis,morus - Fix and simplify CPUID checks

2018-08-06 Thread Milan Broz
On 03/08/18 13:37, Ondrej Mosnacek wrote:
> It turns out I had misunderstood how the x86_match_cpu() function works.
> It evaluates a logical OR of the matching conditions, not logical AND.
> This caused the CPU feature checks for AEGIS to pass even if only SSE2
> (but not AES-NI) was supported (or vice versa), leading to potential
> crashes if something tried to use the registered algs.
> 
> This patch switches the checks to a simpler method that is used e.g. in
> the Camellia x86 code.
> 
> The patch also removes the MODULE_DEVICE_TABLE declarations which
> actually seem to cause the modules to be auto-loaded at boot, which is
> not desired. The crypto API on-demand module loading is sufficient.
> 
> Fixes: 1d373d4e8e15 ("crypto: x86 - Add optimized AEGIS implementations")
> Fixes: 6ecc9d9ff91f ("crypto: x86 - Add optimized MORUS implementations")
> Signed-off-by: Ondrej Mosnacek 

I tried this patch on x86_64 with AES-NI and also on system with
SSE but without AES-NI and it works as expected now
(module is loaded only on demand and optimized one is used if available).

If it is worth it, add
Tested-by: Milan Broz 

Any chance it could still reach 4.18?

Without this patch it actually crashes kernel on x86_64 without AES-NI
but with SSE flags, see https://bugzilla.redhat.com/show_bug.cgi?id=1610180#c4

Thanks,
Milan


[PATCH v2] crypto: x86/aegis,morus - Fix and simplify CPUID checks

2018-08-03 Thread Ondrej Mosnacek
It turns out I had misunderstood how the x86_match_cpu() function works.
It evaluates a logical OR of the matching conditions, not logical AND.
This caused the CPU feature checks for AEGIS to pass even if only SSE2
(but not AES-NI) was supported (or vice versa), leading to potential
crashes if something tried to use the registered algs.

This patch switches the checks to a simpler method that is used e.g. in
the Camellia x86 code.

The patch also removes the MODULE_DEVICE_TABLE declarations which
actually seem to cause the modules to be auto-loaded at boot, which is
not desired. The crypto API on-demand module loading is sufficient.

Fixes: 1d373d4e8e15 ("crypto: x86 - Add optimized AEGIS implementations")
Fixes: 6ecc9d9ff91f ("crypto: x86 - Add optimized MORUS implementations")
Signed-off-by: Ondrej Mosnacek 
---
 arch/x86/crypto/aegis128-aesni-glue.c  | 12 
 arch/x86/crypto/aegis128l-aesni-glue.c | 12 
 arch/x86/crypto/aegis256-aesni-glue.c  | 12 
 arch/x86/crypto/morus1280-avx2-glue.c  | 10 +++---
 arch/x86/crypto/morus1280-sse2-glue.c  | 10 +++---
 arch/x86/crypto/morus640-sse2-glue.c   | 10 +++---
 6 files changed, 21 insertions(+), 45 deletions(-)

diff --git a/arch/x86/crypto/aegis128-aesni-glue.c 
b/arch/x86/crypto/aegis128-aesni-glue.c
index 5de7c0d46edf..acd11b3bf639 100644
--- a/arch/x86/crypto/aegis128-aesni-glue.c
+++ b/arch/x86/crypto/aegis128-aesni-glue.c
@@ -375,16 +375,12 @@ static struct aead_alg crypto_aegis128_aesni_alg[] = {
}
 };
 
-static const struct x86_cpu_id aesni_cpu_id[] = {
-   X86_FEATURE_MATCH(X86_FEATURE_AES),
-   X86_FEATURE_MATCH(X86_FEATURE_XMM2),
-   {}
-};
-MODULE_DEVICE_TABLE(x86cpu, aesni_cpu_id);
-
 static int __init crypto_aegis128_aesni_module_init(void)
 {
-   if (!x86_match_cpu(aesni_cpu_id))
+   if (!boot_cpu_has(X86_FEATURE_XMM2) ||
+   !boot_cpu_has(X86_FEATURE_AES) ||
+   !boot_cpu_has(X86_FEATURE_OSXSAVE) ||
+   !cpu_has_xfeatures(XFEATURE_MASK_SSE, NULL))
return -ENODEV;
 
return crypto_register_aeads(crypto_aegis128_aesni_alg,
diff --git a/arch/x86/crypto/aegis128l-aesni-glue.c 
b/arch/x86/crypto/aegis128l-aesni-glue.c
index 876e4866e633..2071c3d1ae07 100644
--- a/arch/x86/crypto/aegis128l-aesni-glue.c
+++ b/arch/x86/crypto/aegis128l-aesni-glue.c
@@ -375,16 +375,12 @@ static struct aead_alg crypto_aegis128l_aesni_alg[] = {
}
 };
 
-static const struct x86_cpu_id aesni_cpu_id[] = {
-   X86_FEATURE_MATCH(X86_FEATURE_AES),
-   X86_FEATURE_MATCH(X86_FEATURE_XMM2),
-   {}
-};
-MODULE_DEVICE_TABLE(x86cpu, aesni_cpu_id);
-
 static int __init crypto_aegis128l_aesni_module_init(void)
 {
-   if (!x86_match_cpu(aesni_cpu_id))
+   if (!boot_cpu_has(X86_FEATURE_XMM2) ||
+   !boot_cpu_has(X86_FEATURE_AES) ||
+   !boot_cpu_has(X86_FEATURE_OSXSAVE) ||
+   !cpu_has_xfeatures(XFEATURE_MASK_SSE, NULL))
return -ENODEV;
 
return crypto_register_aeads(crypto_aegis128l_aesni_alg,
diff --git a/arch/x86/crypto/aegis256-aesni-glue.c 
b/arch/x86/crypto/aegis256-aesni-glue.c
index 2b5dd3af8f4d..b5f2a8fd5a71 100644
--- a/arch/x86/crypto/aegis256-aesni-glue.c
+++ b/arch/x86/crypto/aegis256-aesni-glue.c
@@ -375,16 +375,12 @@ static struct aead_alg crypto_aegis256_aesni_alg[] = {
}
 };
 
-static const struct x86_cpu_id aesni_cpu_id[] = {
-   X86_FEATURE_MATCH(X86_FEATURE_AES),
-   X86_FEATURE_MATCH(X86_FEATURE_XMM2),
-   {}
-};
-MODULE_DEVICE_TABLE(x86cpu, aesni_cpu_id);
-
 static int __init crypto_aegis256_aesni_module_init(void)
 {
-   if (!x86_match_cpu(aesni_cpu_id))
+   if (!boot_cpu_has(X86_FEATURE_XMM2) ||
+   !boot_cpu_has(X86_FEATURE_AES) ||
+   !boot_cpu_has(X86_FEATURE_OSXSAVE) ||
+   !cpu_has_xfeatures(XFEATURE_MASK_SSE, NULL))
return -ENODEV;
 
return crypto_register_aeads(crypto_aegis256_aesni_alg,
diff --git a/arch/x86/crypto/morus1280-avx2-glue.c 
b/arch/x86/crypto/morus1280-avx2-glue.c
index f111f36d26dc..6634907d6ccd 100644
--- a/arch/x86/crypto/morus1280-avx2-glue.c
+++ b/arch/x86/crypto/morus1280-avx2-glue.c
@@ -37,15 +37,11 @@ asmlinkage void crypto_morus1280_avx2_final(void *state, 
void *tag_xor,
 
 MORUS1280_DECLARE_ALGS(avx2, "morus1280-avx2", 400);
 
-static const struct x86_cpu_id avx2_cpu_id[] = {
-X86_FEATURE_MATCH(X86_FEATURE_AVX2),
-{}
-};
-MODULE_DEVICE_TABLE(x86cpu, avx2_cpu_id);
-
 static int __init crypto_morus1280_avx2_module_init(void)
 {
-   if (!x86_match_cpu(avx2_cpu_id))
+   if (!boot_cpu_has(X86_FEATURE_AVX2) ||
+   !boot_cpu_has(X86_FEATURE_OSXSAVE) ||
+   !cpu_has_xfeatures(XFEATURE_MASK_SSE | XFEATURE_MASK_YMM, NULL))
return -ENODEV;
 
return crypto_register_aeads(crypto_morus1280_avx2_algs,
diff --git a/arch/x86/crypto/morus1280-sse2-glue.c 
b/arch/x86/crypto/morus1280-sse2-glue.c
index 839270aa713c..95cf857d2cbb 10