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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Nov 2021 07:43:43 -0700
From:   Dave Hansen <dave.hansen@...el.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>, linux-kernel@...r.kernel.org
Cc:     "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        "open list:CRYPTO API" <linux-crypto@...r.kernel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Borislav Petkov <bp@...en8.de>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Ingo Molnar <mingo@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        "H. Peter Anvin" <hpa@...or.com>,
        Tim Chen <tim.c.chen@...ux.intel.com>
Subject: Re: [PATCH] crypto: x86/aes-ni: fix AVX detection

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.