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

[Huang, Kai]

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.

Actually I am not clear about this topic.

So the rule is KVM should never adjust CPUID entries passed from userspace?

What if the userspace passed the incorrect CPUID entries?  Should KVM sanitize
those CPUID entries to ensure there's no insane configuration?  My concern is if
we allow guest to be created with insane CPUID configurations, the guest can be
confused and behaviour unexpectedly.

> 
> 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.

I wouldn't worry about userspace being broken, because, IIUC, such broken can
only happen when userspace doesn't do the right thing (i.e. it sets SGX CPUID
0x12,0x1 to have more bits than the XCR0).

> 
> Compile tested only (don't have an SGX system these days).
> 
> 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.
> 
> 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;
> 
> 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
>