| Message ID | 20180802091459.28358-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: x86/aegis - Fix CPUID checks | expand |
On Thu, Aug 2, 2018 at 11:17 AM Ondrej Mosnacek <omosnace@redhat.com> 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 to pass even if only SSE2 (but not > AES-NI) was supported (ir vice versa), leading to potential crashes if > something tried to use the registered algs. > > This patch fixes the checks to ensure that both required CPU features > are supported. The MODULE_DEVICE_TABLE declaration is specified only for > the AES-NI feature array, because I'm not sure what having multiple such > declarations would cause and I believe it should suffice to match on the > more important feature at the alias level and let the init routine do > the full check. > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > Hi Herbert, > > this patch fixes the CPU checks of the AEGIS AES-NI/SSE2 implementations > that have been introduced in 4.18. Once reviewed, it should go to Linus' > tree since it may cause crashes on some systems if the corresponding > configs are enabled. > > @x86 folks, please take a look if I use the MODULE_DEVICE_TABLE macro > correctly here (I'm not sure how to properly declare that the module > needs two CPU features to be both supported; all other modules I saw had > either only single match rule or required just one of the rules to > match). Never mind, I realized MODULE_DEVICE_TABLE isn't actually needed at all here (most modules in arch/x86/crypto don't even use it) and IIUC it will cause the modules to load automatically on boot (which isn't desirable for these new algorithms). I'll send a v2 of the patch that converts both AEGIS and MORUS checks to what most other modules use (e.g. the Camellia x86 optimizations). > > Thanks, > > Ondrej Mosnacek > > arch/x86/crypto/aegis128-aesni-glue.c | 8 ++++++-- > arch/x86/crypto/aegis128l-aesni-glue.c | 8 ++++++-- > arch/x86/crypto/aegis256-aesni-glue.c | 8 ++++++-- > 3 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/crypto/aegis128-aesni-glue.c b/arch/x86/crypto/aegis128-aesni-glue.c > index 5de7c0d46edf..6a5abed59593 100644 > --- a/arch/x86/crypto/aegis128-aesni-glue.c > +++ b/arch/x86/crypto/aegis128-aesni-glue.c > @@ -377,14 +377,18 @@ 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 const struct x86_cpu_id sse2_cpu_id[] = { > + X86_FEATURE_MATCH(X86_FEATURE_XMM2), > + {} > +}; > + > static int __init crypto_aegis128_aesni_module_init(void) > { > - if (!x86_match_cpu(aesni_cpu_id)) > + if (!x86_match_cpu(aesni_cpu_id) || !x86_match_cpu(sse2_cpu_id)) > 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..691c52701c2d 100644 > --- a/arch/x86/crypto/aegis128l-aesni-glue.c > +++ b/arch/x86/crypto/aegis128l-aesni-glue.c > @@ -377,14 +377,18 @@ 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 const struct x86_cpu_id sse2_cpu_id[] = { > + X86_FEATURE_MATCH(X86_FEATURE_XMM2), > + {} > +}; > + > static int __init crypto_aegis128l_aesni_module_init(void) > { > - if (!x86_match_cpu(aesni_cpu_id)) > + if (!x86_match_cpu(aesni_cpu_id) || !x86_match_cpu(sse2_cpu_id)) > 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..481b5e4f4fd0 100644 > --- a/arch/x86/crypto/aegis256-aesni-glue.c > +++ b/arch/x86/crypto/aegis256-aesni-glue.c > @@ -377,14 +377,18 @@ 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 const struct x86_cpu_id sse2_cpu_id[] = { > + X86_FEATURE_MATCH(X86_FEATURE_XMM2), > + {} > +}; > + > static int __init crypto_aegis256_aesni_module_init(void) > { > - if (!x86_match_cpu(aesni_cpu_id)) > + if (!x86_match_cpu(aesni_cpu_id) || !x86_match_cpu(sse2_cpu_id)) > return -ENODEV; > > return crypto_register_aeads(crypto_aegis256_aesni_alg, > -- > 2.17.1 >
diff --git a/arch/x86/crypto/aegis128-aesni-glue.c b/arch/x86/crypto/aegis128-aesni-glue.c index 5de7c0d46edf..6a5abed59593 100644 --- a/arch/x86/crypto/aegis128-aesni-glue.c +++ b/arch/x86/crypto/aegis128-aesni-glue.c @@ -377,14 +377,18 @@ 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 const struct x86_cpu_id sse2_cpu_id[] = { + X86_FEATURE_MATCH(X86_FEATURE_XMM2), + {} +}; + static int __init crypto_aegis128_aesni_module_init(void) { - if (!x86_match_cpu(aesni_cpu_id)) + if (!x86_match_cpu(aesni_cpu_id) || !x86_match_cpu(sse2_cpu_id)) 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..691c52701c2d 100644 --- a/arch/x86/crypto/aegis128l-aesni-glue.c +++ b/arch/x86/crypto/aegis128l-aesni-glue.c @@ -377,14 +377,18 @@ 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 const struct x86_cpu_id sse2_cpu_id[] = { + X86_FEATURE_MATCH(X86_FEATURE_XMM2), + {} +}; + static int __init crypto_aegis128l_aesni_module_init(void) { - if (!x86_match_cpu(aesni_cpu_id)) + if (!x86_match_cpu(aesni_cpu_id) || !x86_match_cpu(sse2_cpu_id)) 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..481b5e4f4fd0 100644 --- a/arch/x86/crypto/aegis256-aesni-glue.c +++ b/arch/x86/crypto/aegis256-aesni-glue.c @@ -377,14 +377,18 @@ 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 const struct x86_cpu_id sse2_cpu_id[] = { + X86_FEATURE_MATCH(X86_FEATURE_XMM2), + {} +}; + static int __init crypto_aegis256_aesni_module_init(void) { - if (!x86_match_cpu(aesni_cpu_id)) + if (!x86_match_cpu(aesni_cpu_id) || !x86_match_cpu(sse2_cpu_id)) return -ENODEV; return crypto_register_aeads(crypto_aegis256_aesni_alg,
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 to pass even if only SSE2 (but not AES-NI) was supported (ir vice versa), leading to potential crashes if something tried to use the registered algs. This patch fixes the checks to ensure that both required CPU features are supported. The MODULE_DEVICE_TABLE declaration is specified only for the AES-NI feature array, because I'm not sure what having multiple such declarations would cause and I believe it should suffice to match on the more important feature at the alias level and let the init routine do the full check. Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- Hi Herbert, this patch fixes the CPU checks of the AEGIS AES-NI/SSE2 implementations that have been introduced in 4.18. Once reviewed, it should go to Linus' tree since it may cause crashes on some systems if the corresponding configs are enabled. @x86 folks, please take a look if I use the MODULE_DEVICE_TABLE macro correctly here (I'm not sure how to properly declare that the module needs two CPU features to be both supported; all other modules I saw had either only single match rule or required just one of the rules to match). Thanks, Ondrej Mosnacek arch/x86/crypto/aegis128-aesni-glue.c | 8 ++++++-- arch/x86/crypto/aegis128l-aesni-glue.c | 8 ++++++-- arch/x86/crypto/aegis256-aesni-glue.c | 8 ++++++-- 3 files changed, 18 insertions(+), 6 deletions(-)