On Thu, Aug 2, 2018 at 11:17 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
