linux-kernel - Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

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, 5 Apr 2023 03:05:10 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> *** WARNING *** ABI breakage.
> 
> Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0)
> for SGX enclaves.  Past me didn't understand the roles and responsibilities
> between userspace and KVM with respect to CPUID leafs, i.e. I thought I was
> being helpful by having KVM adjust the entries.
> 
> This is clearly an ABI breakage, but QEMU (tries to) do the right thing,
> and AFAIK no other VMMs support SGX (yet), so I'm hoping we can excise the
> ugly before userspace starts depending on the bad behavior.
> 
> Compile tested only (don't have an SGX system these days).

I'll look into this, and at the meantime ...

> 
> Note, QEMU commit 301e90675c ("target/i386: Enable support for XSAVES
> based features") completely broke SGX by using allowed XSS instead of
> XCR0, and no one has complained.  That gives me hope that this one will
> go through as well.

...

Actually we got complain around half year ago:

https://github.com/gramineproject/gramine/issues/955#issuecomment-1272829510

> 
> I belive the QEMU fix is below.  I'll post a patch at some point unless
> someone wants to do the dirty work and claim the patch as their own.
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 6576287e5b..f083ff4335 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5718,8 +5718,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          } else {
>              *eax &= env->features[FEAT_SGX_12_1_EAX];
>              *ebx &= 0; /* ebx reserve */
> -            *ecx &= env->features[FEAT_XSAVE_XSS_LO];
> -            *edx &= env->features[FEAT_XSAVE_XSS_HI];
> +            *ecx &= env->features[FEAT_XSAVE_XCR0_LO];
> +            *edx &= env->features[FEAT_XSAVE_XCR0_HI];
>  
>              /* FP and SSE are always allowed regardless of XSAVE/XCR0. */
>              *ecx |= XSTATE_FP_MASK | XSTATE_SSE_MASK;

And since then Yang posted a patch to Qemu mailing list to fix:

https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg04990.html

I thought it had been merged, but it seems it hasn't :)

> 
> Sean Christopherson (3):
>   KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for
>     ECREATE
>   KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)
>   KVM: x86: Open code supported XCR0 calculation in
>     kvm_vcpu_after_set_cpuid()
> 
>  arch/x86/kvm/cpuid.c   | 43 ++++++++++--------------------------------
>  arch/x86/kvm/vmx/sgx.c |  3 ++-
>  2 files changed, 12 insertions(+), 34 deletions(-)
> 
> 
> base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393
> -- 
> 2.40.0.348.gf938b09366-goog
>