KVM: x86: SGX vs. XCR0 cleanups

Explicitly check the vCPU's supported XCR0 when determining whether or not the XFRM for ECREATE is valid. Checking CPUID works because KVM updates guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed XCR0, but that is rather subtle and KVM should not modify guest CPUID except for modeling true runtime behavior (allowed XFRM is most definitely not "runtime" behavior).

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kvm/vmx/sgx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx/sgx.c +0 -0

index aa53c98034bf..362a31b19b0e 100644

arch/x86/kvm/vmx/sgx.c +0 -0

 		kvm_inject_gp(vcpu, 0);
 		return 1;
 	}

Signature · 1 line

2.40.0.348.gf938b09366-goog

Quoted 17 lines · Sean Christopherson

Explicitly check the vCPU's supported XCR0 when determining whether or not the XFRM for ECREATE is valid. Checking CPUID works because KVM updates guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed XCR0, but that is rather subtle and KVM should not modify guest CPUID except for modeling true runtime behavior (allowed XFRM is most definitely not "runtime" behavior).

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kvm/vmx/sgx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx/sgx.c +0 -0

index aa53c98034bf..362a31b19b0e 100644

arch/x86/kvm/vmx/sgx.c static int __handle_encls_ecreate(struct kvm_vcpu *vcpu, · line 179

Quoted 6 lines

arch/x86/kvm/vmx/sgx.c +2 -1

@@ -175,7 +175,8 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
 	    (u32)attributes & ~sgx_12_1->eax ||
 	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
 	    (u32)xfrm & ~sgx_12_1->ecx ||
-	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
+	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
+	    xfrm & ~vcpu->arch.guest_supported_xcr0) {

Perhaps this change is needed even without patch 2?

This is because when CPUID 0xD doesn't exist, guest_supported_xcr0 is 0. But when CPUID 0xD doesn't exist, IIUC currently KVM doesn't clear SGX in CPUID, and sgx_12_1->ecx is always set to 0x3.

__handle_encls_ereate() doesn't check CPUID 0xD either, so w/o above explicit check xfrm against guest_supported_xcr0, it seems guest can successfully run ECREATE when it doesn't have CPUID 0xD?

Quoted 27 lines · Kai

Quoted 17 lines · Sean Christopherson

Explicitly check the vCPU's supported XCR0 when determining whether or not the XFRM for ECREATE is valid. Checking CPUID works because KVM updates guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed XCR0, but that is rather subtle and KVM should not modify guest CPUID except for modeling true runtime behavior (allowed XFRM is most definitely not "runtime" behavior).

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kvm/vmx/sgx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx/sgx.c +0 -0

index aa53c98034bf..362a31b19b0e 100644

arch/x86/kvm/vmx/sgx.c static int __handle_encls_ecreate(struct kvm_vcpu *vcpu, · line 179

Quoted 6 lines

arch/x86/kvm/vmx/sgx.c +2 -1

@@ -175,7 +175,8 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
 	    (u32)attributes & ~sgx_12_1->eax ||
 	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
 	    (u32)xfrm & ~sgx_12_1->ecx ||
-	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
+	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
+	    xfrm & ~vcpu->arch.guest_supported_xcr0) {

Perhaps this change is needed even without patch 2?

Quoted 3 lines

This is because when CPUID 0xD doesn't exist, guest_supported_xcr0 is 0. But when CPUID 0xD doesn't exist, IIUC currently KVM doesn't clear SGX in CPUID, and sgx_12_1->ecx is always set to 0x3.

Hrm, that's a bug in this patch. Drat. More below.

Quoted 3 lines

__handle_encls_ereate() doesn't check CPUID 0xD either, so w/o above explicit check xfrm against guest_supported_xcr0, it seems guest can successfully run ECREATE when it doesn't have CPUID 0xD?

ECREATE doesn't have a strict dependency on CPUID 0xD or XSAVE. This exact scenario is called out in the SDM:

Legal values for SECS.ATTRIBUTES.XFRM conform to these requirements:
* XFRM[1:0] must be set to 0x3.
* If the processor does support XSAVE, XFRM must contain a value that would be legal if loaded into XCR0.
* If the processor does not support XSAVE, or if the system software has not enabled XSAVE, then XFRM[63:2] must be zero.

So the above needs to be either

	xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE)

or

	(xfrm & ~XFEATURE_MASK_FPSSE & ~vcpu->arch.guest_supported_xcr0)

I think I prefer the first one as I find it slightly more obvious that FP+SSE are allowed in addition to the XCR0 bits.

Quoted 54 lines · Sean Christopherson

Quoted 27 lines · Kai

Quoted 17 lines · Sean Christopherson

Explicitly check the vCPU's supported XCR0 when determining whether or not the XFRM for ECREATE is valid. Checking CPUID works because KVM updates guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed XCR0, but that is rather subtle and KVM should not modify guest CPUID except for modeling true runtime behavior (allowed XFRM is most definitely not "runtime" behavior).

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kvm/vmx/sgx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx/sgx.c +0 -0

index aa53c98034bf..362a31b19b0e 100644

arch/x86/kvm/vmx/sgx.c static int __handle_encls_ecreate(struct kvm_vcpu *vcpu, · line 179

Quoted 6 lines

arch/x86/kvm/vmx/sgx.c +2 -1

@@ -175,7 +175,8 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
 	    (u32)attributes & ~sgx_12_1->eax ||
 	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
 	    (u32)xfrm & ~sgx_12_1->ecx ||
-	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
+	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
+	    xfrm & ~vcpu->arch.guest_supported_xcr0) {

Perhaps this change is needed even without patch 2?

Quoted 3 lines

This is because when CPUID 0xD doesn't exist, guest_supported_xcr0 is 0. But when CPUID 0xD doesn't exist, IIUC currently KVM doesn't clear SGX in CPUID, and sgx_12_1->ecx is always set to 0x3.

Hrm, that's a bug in this patch. Drat. More below.

Quoted 3 lines

__handle_encls_ereate() doesn't check CPUID 0xD either, so w/o above explicit check xfrm against guest_supported_xcr0, it seems guest can successfully run ECREATE when it doesn't have CPUID 0xD?

ECREATE doesn't have a strict dependency on CPUID 0xD or XSAVE. This exact scenario is called out in the SDM:

Legal values for SECS.ATTRIBUTES.XFRM conform to these requirements:
* XFRM[1:0] must be set to 0x3.
* If the processor does support XSAVE, XFRM must contain a value that would be legal if loaded into XCR0.
* If the processor does not support XSAVE, or if the system software has not enabled XSAVE, then XFRM[63:2] must be zero.

So the above needs to be either

	xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE)

or

Quoted 5 lines

	(xfrm & ~XFEATURE_MASK_FPSSE & ~vcpu->arch.guest_supported_xcr0)

I think I prefer the first one as I find it slightly more obvious that FP+SSE are allowed in addition to the XCR0 bits.

The above check doesn't verify xfrm is a super set of 0x3. I think we verify that per SDM:

39.7.3 Processor Extended States and ENCLS[ECREATE]

The ECREATE leaf function of the ENCLS instruction enforces a number of consistency checks described earlier. The execution of ENCLS[ECREATE] leaf function results in a #GP(0) in any of the following cases:
• SECS.ATTRIBUTES.XFRM[1:0] is not 3.
• The processor does not support XSAVE and any of the following is true:
— SECS.ATTRIBUTES.XFRM[63:2] is not 0.
— SECS.SSAFRAMESIZE is 0.
• The processor supports XSAVE and any of the following is true:
— XSETBV would fault on an attempt to load XFRM into XCR0. — XFRM[63]=1.
— The SSAFRAME is too small to hold required, enabled states ...

And in the ECREATE pseudo code, the relevant parts seem to be:

(* Check lower 2 bits of XFRM are set *)
IF ( ( DS:TMP_SECS.ATTRIBUTES.XFRM BitwiseAND 03H) ≠ 03H)
THEN #GP(0); FI;

IF (XFRM is illegal)
THEN #GP(0); FI;

The first part is clear, but the second part is vague.

I am not sure in hardware behaviour, whether XCR0 is actually checked in ECREATE. It's more likely XCRO is actually checked in EENTER.

But I think it's just fine to also check against XCR0 here.

Quoted 61 lines · Kai

Quoted 59 lines · Sean Christopherson

Quoted 27 lines · Kai

Quoted 17 lines · Sean Christopherson

Explicitly check the vCPU's supported XCR0 when determining whether or not the XFRM for ECREATE is valid. Checking CPUID works because KVM updates guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed XCR0, but that is rather subtle and KVM should not modify guest CPUID except for modeling true runtime behavior (allowed XFRM is most definitely not "runtime" behavior).

Signed-off-by Sean Christopherson <[email protected]>

---
 arch/x86/kvm/vmx/sgx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx/sgx.c +0 -0

index aa53c98034bf..362a31b19b0e 100644

arch/x86/kvm/vmx/sgx.c static int __handle_encls_ecreate(struct kvm_vcpu *vcpu, · line 179

Quoted 6 lines

arch/x86/kvm/vmx/sgx.c +2 -1

@@ -175,7 +175,8 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
 	    (u32)attributes & ~sgx_12_1->eax ||
 	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
 	    (u32)xfrm & ~sgx_12_1->ecx ||
-	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
+	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
+	    xfrm & ~vcpu->arch.guest_supported_xcr0) {

Perhaps this change is needed even without patch 2?

Quoted 3 lines

This is because when CPUID 0xD doesn't exist, guest_supported_xcr0 is 0. But when CPUID 0xD doesn't exist, IIUC currently KVM doesn't clear SGX in CPUID, and sgx_12_1->ecx is always set to 0x3.

Hrm, that's a bug in this patch. Drat. More below.

Quoted 3 lines

__handle_encls_ereate() doesn't check CPUID 0xD either, so w/o above explicit check xfrm against guest_supported_xcr0, it seems guest can successfully run ECREATE when it doesn't have CPUID 0xD?

ECREATE doesn't have a strict dependency on CPUID 0xD or XSAVE. This exact scenario is called out in the SDM:

Legal values for SECS.ATTRIBUTES.XFRM conform to these requirements:
* XFRM[1:0] must be set to 0x3.
* If the processor does support XSAVE, XFRM must contain a value that would be legal if loaded into XCR0.
* If the processor does not support XSAVE, or if the system software has not enabled XSAVE, then XFRM[63:2] must be zero.

So the above needs to be either

	xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE)

or

	(xfrm & ~XFEATURE_MASK_FPSSE & ~vcpu->arch.guest_supported_xcr0)

I think I prefer the first one as I find it slightly more obvious that FP+SSE are allowed in addition to the XCR0 bits.

Quoted 2 lines

The above check doesn't verify xfrm is a super set of 0x3. I think we verify that per SDM:

Oooh, right. It's not that FP+SSE are always allowed, it's that FP+SSE must always be _set_. So this?

		xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE) ||
		(xfrm & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE

Quoted 27 lines

39.7.3 Processor Extended States and ENCLS[ECREATE]

The ECREATE leaf function of the ENCLS instruction enforces a number of consistency checks described earlier. The execution of ENCLS[ECREATE] leaf function results in a #GP(0) in any of the following cases:
• SECS.ATTRIBUTES.XFRM[1:0] is not 3.
• The processor does not support XSAVE and any of the following is true:
— SECS.ATTRIBUTES.XFRM[63:2] is not 0.
— SECS.SSAFRAMESIZE is 0.
• The processor supports XSAVE and any of the following is true:
— XSETBV would fault on an attempt to load XFRM into XCR0. — XFRM[63]=1.
— The SSAFRAME is too small to hold required, enabled states ...

And in the ECREATE pseudo code, the relevant parts seem to be:

(* Check lower 2 bits of XFRM are set *)
IF ( ( DS:TMP_SECS.ATTRIBUTES.XFRM BitwiseAND 03H) ≠ 03H)
THEN #GP(0); FI;

IF (XFRM is illegal)
THEN #GP(0); FI;

The first part is clear, but the second part is vague.

Quoted 4 lines

I am not sure in hardware behaviour, whether XCR0 is actually checked in ECREATE. It's more likely XCRO is actually checked in EENTER.

But I think it's just fine to also check against XCR0 here.

ECREATE doesn't check XCR0, it checks that XFRM represents a legal XCR0 values for the platform, which in KVM is tracked as guest_supported_xcr0.

Quoted 5 lines

Oooh, right. It's not that FP+SSE are always allowed, it's that FP+SSE must always be _set_. So this?

Quoted 2 lines

		xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE) ||
		(xfrm & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE

Looks good.

I'll try to get some test done with this code change.

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

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.

target/i386/cpu.c +0 -0

index 6576287e5b..f083ff4335 100644

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

Signature · 1 line

2.40.0.348.gf938b09366-goog

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

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.

target/i386/cpu.c +2 -2

index 6576287e5b..f083ff4335 100644
@@ -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

Signature · 1 line

2.40.0.348.gf938b09366-goog

Quoted 6 lines · Sean Christopherson

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

Quoted 11 lines · Kai

Quoted 6 lines · Sean Christopherson

*** 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?

Yes, except for true runtime entries where a CPUID leaf is dynamic based on other CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.

Quoted 4 lines

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.

It is userspace's responsibility to provide a sane, correct setup. The one exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an unsupported virtual address width, the argument being that a malicious userspace could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a VMCS field.

The reason for KVM punting to userspace is that it's all but impossible to define what is/isn't sane. A really good example would be an alternative we (Google) considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly miss reserved bit #PFs.

Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in anticipation of eventual migration. So long as userspace doesn't actually enumerate memslots in the illegal address space, KVM would be able to treat such accesses as emulated MMIO, and would only need to intercept #PF(RSVD).

Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR definitely qualifies as insane since it really can't work correctly, but in our opinion it was far superior to running with allow_smaller_maxphyaddr=true.

And sane is not the same thing as architecturally legal. AMX is a good example of this. It's _technically_ legal to enumerate support for XFEATURE_TILE_CFG but not XFEATURE_TILE_DATA in CPUID, but illegal to actually try to enable TILE_CFG in XCR0 without also enabling TILE_DATA. KVM should arguably reject CPUID configs with TILE_CFG but not TILE_DATA, and vice versa, but then KVM is rejecting a 100% architecturally valid, if insane, CPUID configuration. Ditto for nearly all of the VMX control bits versus their CPUID counterparts.

And sometimes there are good reasons to run a VM with a truly insane configuration, e.g. for testing purposes.

TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.

Quoted 51 lines · On Wed, 5 Apr 2023 19:10:40 -0700 Sean Christopherson <[email protected]> wrote:

Quoted 11 lines · Kai

Quoted 6 lines · Sean Christopherson

*** 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?

Yes, except for true runtime entries where a CPUID leaf is dynamic based on other CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.

Quoted 4 lines

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.

It is userspace's responsibility to provide a sane, correct setup. The one exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an unsupported virtual address width, the argument being that a malicious userspace could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a VMCS field.

The reason for KVM punting to userspace is that it's all but impossible to define what is/isn't sane. A really good example would be an alternative we (Google) considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly miss reserved bit #PFs.

Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in anticipation of eventual migration. So long as userspace doesn't actually enumerate memslots in the illegal address space, KVM would be able to treat such accesses as emulated MMIO, and would only need to intercept #PF(RSVD).

Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR definitely qualifies as insane since it really can't work correctly, but in our opinion it was far superior to running with allow_smaller_maxphyaddr=true.

And sane is not the same thing as architecturally legal. AMX is a good example of this. It's _technically_ legal to enumerate support for XFEATURE_TILE_CFG but not XFEATURE_TILE_DATA in CPUID, but illegal to actually try to enable TILE_CFG in XCR0 without also enabling TILE_DATA. KVM should arguably reject CPUID configs with TILE_CFG but not TILE_DATA, and vice versa, but then KVM is rejecting a 100% architecturally valid, if insane, CPUID configuration. Ditto for nearly all of the VMX control bits versus their CPUID counterparts.

Quoted 4 lines

And sometimes there are good reasons to run a VM with a truly insane configuration, e.g. for testing purposes.

TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.

Interesting point. I was digging the CPUID virtualization OF TDX/SNP. It would be nice to have a conclusion of what is "sane" and what is the proper role for KVM, as firmware/TDX module is going to validate the "sane" CPUID.

TDX/SNP requires the CPUID to be pre-configured and validated before creating a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in SNP_LAUNCH_UPDATE in SNP).

IIUC according to what you mentioned, KVM should be treated like "CPUID box"
for QEMU and the checks in KVM is only to ensure the requirements of a chosen
one is literally possible and correct. KVM should not care if the combination, the usage of the chosen ones is insane or not, which gives QEMU flexibility.

As the valid CPUIDs have been decided when creating a CC guest, what should be the proper behavior (basically any new checks?) of KVM for the later SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced at least, because some KVM code paths rely on guest CPUID configuration.

Quoted 19 lines · Zhi Wang

Quoted 1 line · On Wed, 5 Apr 2023 19:10:40 -0700 Sean Christopherson <[email protected]> wrote:

TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.

Interesting point. I was digging the CPUID virtualization OF TDX/SNP. It would be nice to have a conclusion of what is "sane" and what is the proper role for KVM, as firmware/TDX module is going to validate the "sane" CPUID.

TDX/SNP requires the CPUID to be pre-configured and validated before creating a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in SNP_LAUNCH_UPDATE in SNP).

IIUC according to what you mentioned, KVM should be treated like "CPUID box" for QEMU and the checks in KVM is only to ensure the requirements of a chosen one is literally possible and correct. KVM should not care if the combination, the usage of the chosen ones is insane or not, which gives QEMU flexibility.

Quoted 4 lines

As the valid CPUIDs have been decided when creating a CC guest, what should be the proper behavior (basically any new checks?) of KVM for the later SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced at least, because some KVM code paths rely on guest CPUID configuration.

For TDX guest my preference is KVM to save all CPUID entries in TDH.MNG.INIT and manually make vcpu's CPUID point to the saved CPUIDs. And then KVM just ignore the SET_CPUID2 for TDX guest.

Quoted 25 lines · Kai Huang

Quoted 23 lines · Zhi Wang

Quoted 1 line · On Wed, 5 Apr 2023 19:10:40 -0700 Sean Christopherson <[email protected]> wrote:

TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.

Interesting point. I was digging the CPUID virtualization OF TDX/SNP. It would be nice to have a conclusion of what is "sane" and what is the proper role for KVM, as firmware/TDX module is going to validate the "sane" CPUID.

TDX/SNP requires the CPUID to be pre-configured and validated before creating a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in SNP_LAUNCH_UPDATE in SNP).

IIUC according to what you mentioned, KVM should be treated like "CPUID box" for QEMU and the checks in KVM is only to ensure the requirements of a chosen one is literally possible and correct. KVM should not care if the combination, the usage of the chosen ones is insane or not, which gives QEMU flexibility.

As the valid CPUIDs have been decided when creating a CC guest, what should be the proper behavior (basically any new checks?) of KVM for the later SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced at least, because some KVM code paths rely on guest CPUID configuration.

Quoted 3 lines

For TDX guest my preference is KVM to save all CPUID entries in TDH.MNG.INIT and manually make vcpu's CPUID point to the saved CPUIDs. And then KVM just ignore the SET_CPUID2 for TDX guest.

It's been a long while since I looked at TDX's CPUID management, but IIRC ignoring SET_CPUID2 is not an option becuase the TDH.MNG.INIT only allows leafs that are known to the TDX Module, e.g. KVM's paravirt CPUID leafs can't be communicated via TDH.MNG.INIT.

KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module could result in previously allowed input becoming invalid.

Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the leaf would effectively make _everything_ valid input. If KVM were to rely solely on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.

Back to Zhi's question, the best thing to do for TDX and SNP is likely to require that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent. The key difference is that KVM would be enforcing consistency, not sanity. I.e. KVM isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring that userspace provide a CPUID model that's consistent with what userspace provided earlier.

Quoted 4 lines · Sean Christopherson

KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module could result in previously allowed input becoming invalid.

How about only filtering out PV related CPUIDs when applying CPUIDs to TDH.MNG.INIT? I think we can assume they are not gonna be known to TDX module anyway.

Quoted 4 lines

Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the leaf would effectively make _everything_ valid input. If KVM were to rely solely on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.

Right. Disallowing SET_CPUID{2} probably is better, as it gives userspace a more concrete result.

Quoted 7 lines

Back to Zhi's question, the best thing to do for TDX and SNP is likely to require that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent. The key difference is that KVM would be enforcing consistency, not sanity. I.e. KVM isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring that userspace provide a CPUID model that's consistent with what userspace provided earlier.

So IIUC, you prefer to verifying the CPUIDs in SET_CPUID{2} are a super set of the CPUIDs provided in TDH.MNG.INIT? And KVM manually verifies all CPUIDs for all vcpus are consistent (the same) in SET_CPUID{2}?

Looks this is over-complicated, _if_ the "only filtering out PV related CPUIDs when applying CPUIDs to TDH.MNG.INIT" approach works.

Quoted 6 lines · Kai Huang

Quoted 4 lines · Sean Christopherson

KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module could result in previously allowed input becoming invalid.

Quoted 3 lines

How about only filtering out PV related CPUIDs when applying CPUIDs to TDH.MNG.INIT? I think we can assume they are not gonna be known to TDX module anyway.

Nope, not going down that road. Fool me once[*], shame on you. Fool me twice, shame on me :-)

Objections to hardware vendors defining PV interfaces aside, there exist leafs that are neither PV related nor known to the TDX module, e.g. Centaur leafs. I think it's extremely unlikely (understatement) that anyone will want to expose Centaur leafs to a TDX guest, but again I want to say out of the business of telling userspace what is and isn't sane CPUID models.

[*] https://lore.kernel.org/all/[email protected]

Quoted 16 lines

Quoted 4 lines

Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the leaf would effectively make _everything_ valid input. If KVM were to rely solely on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.

Right. Disallowing SET_CPUID{2} probably is better, as it gives userspace a more concrete result.

Quoted 7 lines

Back to Zhi's question, the best thing to do for TDX and SNP is likely to require that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent. The key difference is that KVM would be enforcing consistency, not sanity. I.e. KVM isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring that userspace provide a CPUID model that's consistent with what userspace provided earlier.

Quoted 3 lines

So IIUC, you prefer to verifying the CPUIDs in SET_CPUID{2} are a super set of the CPUIDs provided in TDH.MNG.INIT? And KVM manually verifies all CPUIDs for all vcpus are consistent (the same) in SET_CPUID{2}?

Yes, except KVM doesn't need to verify vCPUs are consistent with respect to each other, just that each vCPU is consistent with respect to what was reported to the TDX Module.

Quoted 3 lines

Looks this is over-complicated, _if_ the "only filtering out PV related CPUIDs when applying CPUIDs to TDH.MNG.INIT" approach works.

It's not complicated at all. Walk through the leafs defined during TDH.MNG.INIT, reject KVM_SET_CPUID if a leaf isn't present or doesn't match exactly. Or has the TDX spec changed and it's no longer that simple?

Quoted 7 lines · Sean Christopherson

Quoted 6 lines · Kai Huang

Quoted 4 lines · Sean Christopherson

KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module could result in previously allowed input becoming invalid.

Quoted 6 lines

Quoted 3 lines

How about only filtering out PV related CPUIDs when applying CPUIDs to TDH.MNG.INIT? I think we can assume they are not gonna be known to TDX module anyway.

Nope, not going down that road. Fool me once[*], shame on you. Fool me twice, shame on me :-)

Ah OK :)

Quoted 6 lines

Objections to hardware vendors defining PV interfaces aside, there exist leafs that are neither PV related nor known to the TDX module, e.g. Centaur leafs. I think it's extremely unlikely (understatement) that anyone will want to expose Centaur leafs to a TDX guest, but again I want to say out of the business of telling userspace what is and isn't sane CPUID models.

Right. There might be use case that TDX guest wants to use some CPUID which isn't handled by the TDX module but purely by KVM. We don't want to limit the possibility. Totally agree.

Quoted 23 lines

[*] https://lore.kernel.org/all/[email protected]

Quoted 19 lines

Quoted 4 lines

Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the leaf would effectively make _everything_ valid input. If KVM were to rely solely on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.

Right. Disallowing SET_CPUID{2} probably is better, as it gives userspace a more concrete result.

Quoted 7 lines

Back to Zhi's question, the best thing to do for TDX and SNP is likely to require that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent. The key difference is that KVM would be enforcing consistency, not sanity. I.e. KVM isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring that userspace provide a CPUID model that's consistent with what userspace provided earlier.

So IIUC, you prefer to verifying the CPUIDs in SET_CPUID{2} are a super set of the CPUIDs provided in TDH.MNG.INIT? And KVM manually verifies all CPUIDs for all vcpus are consistent (the same) in SET_CPUID{2}?

Quoted 3 lines

Yes, except KVM doesn't need to verify vCPUs are consistent with respect to each other, just that each vCPU is consistent with respect to what was reported to the TDX Module.

OK. Fine to me.

Quoted 8 lines

Quoted 4 lines

Looks this is over-complicated, _if_ the "only filtering out PV related CPUIDs when applying CPUIDs to TDH.MNG.INIT" approach works.

It's not complicated at all. Walk through the leafs defined during TDH.MNG.INIT, reject KVM_SET_CPUID if a leaf isn't present or doesn't match exactly. Or has the TDX spec changed and it's no longer that simple?

No the module hasn't been changed, and yes it should be as simple as you said. I just had some first impression that handling CPUID in one IOCTL (TDH.MNG.INIT) should be simpler than handling CPUID in two IOCTLs, but I guess this might not be true :)

Anyway I agree with your suggestion. Thanks.

Quoted 68 lines · Zhi Wang

Quoted 51 lines · On Wed, 5 Apr 2023 19:10:40 -0700 Sean Christopherson <[email protected]> wrote:

Quoted 11 lines · Kai

Quoted 6 lines · Sean Christopherson

*** 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?

Yes, except for true runtime entries where a CPUID leaf is dynamic based on other CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.

Quoted 4 lines

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.

It is userspace's responsibility to provide a sane, correct setup. The one exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an unsupported virtual address width, the argument being that a malicious userspace could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a VMCS field.

The reason for KVM punting to userspace is that it's all but impossible to define what is/isn't sane. A really good example would be an alternative we (Google) considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly miss reserved bit #PFs.

Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in anticipation of eventual migration. So long as userspace doesn't actually enumerate memslots in the illegal address space, KVM would be able to treat such accesses as emulated MMIO, and would only need to intercept #PF(RSVD).

Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR definitely qualifies as insane since it really can't work correctly, but in our opinion it was far superior to running with allow_smaller_maxphyaddr=true.

And sane is not the same thing as architecturally legal. AMX is a good example of this. It's _technically_ legal to enumerate support for XFEATURE_TILE_CFG but not XFEATURE_TILE_DATA in CPUID, but illegal to actually try to enable TILE_CFG in XCR0 without also enabling TILE_DATA. KVM should arguably reject CPUID configs with TILE_CFG but not TILE_DATA, and vice versa, but then KVM is rejecting a 100% architecturally valid, if insane, CPUID configuration. Ditto for nearly all of the VMX control bits versus their CPUID counterparts.

Quoted 4 lines

And sometimes there are good reasons to run a VM with a truly insane configuration, e.g. for testing purposes.

TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.

Interesting point. I was digging the CPUID virtualization OF TDX/SNP. It would be nice to have a conclusion of what is "sane" and what is the proper role for KVM, as firmware/TDX module is going to validate the "sane" CPUID.

TDX/SNP requires the CPUID to be pre-configured and validated before creating a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in SNP_LAUNCH_UPDATE in SNP).

Quoted 8 lines

IIUC according to what you mentioned, KVM should be treated like "CPUID box"
for QEMU and the checks in KVM is only to ensure the requirements of a chosen
one is literally possible and correct. KVM should not care if the combination, the usage of the chosen ones is insane or not, which gives QEMU flexibility.

As the valid CPUIDs have been decided when creating a CC guest, what should be the proper behavior (basically any new checks?) of KVM for the later SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced at least, because some KVM code paths rely on guest CPUID configuration.

For TDX guest my preference is KVM to save all CPUID entries in TDH.MNG.INIT and manually make vcpu's CPUID point to the saved CPUIDs. And then KVM just ignore the SET_CPUID2 for TDX guest.

Not sure whether AMD counterpart can be done in similar way though.

Quoted 11 lines · Kai

Quoted 6 lines · Sean Christopherson

*** 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?

Yes, except for true runtime entries where a CPUID leaf is dynamic based on other CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.

Quoted 4 lines

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.

It is userspace's responsibility to provide a sane, correct setup. The one exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an unsupported virtual address width, the argument being that a malicious userspace could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a VMCS field.

The reason for KVM punting to userspace is that it's all but impossible to define what is/isn't sane. A really good example would be an alternative we (Google) considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly miss reserved bit #PFs.

Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in anticipation of eventual migration. So long as userspace doesn't actually enumerate memslots in the illegal address space, KVM would be able to treat such accesses as emulated MMIO, and would only need to intercept #PF(RSVD).

Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR definitely qualifies as insane since it really can't work correctly, but in our opinion it was far superior to running with allow_smaller_maxphyaddr=true.

And sane is not the same thing as architecturally legal. AMX is a good example of this. It's _technically_ legal to enumerate support for XFEATURE_TILE_CFG but not XFEATURE_TILE_DATA in CPUID, but illegal to actually try to enable TILE_CFG in XCR0 without also enabling TILE_DATA. KVM should arguably reject CPUID configs with TILE_CFG but not TILE_DATA, and vice versa, but then KVM is rejecting a 100% architecturally valid, if insane, CPUID configuration. Ditto for nearly all of the VMX control bits versus their CPUID counterparts.

And sometimes there are good reasons to run a VM with a truly insane configuration, e.g. for testing purposes.

TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.

Quoted 21 lines · Sean Christopherson

Quoted 11 lines · Kai

Quoted 6 lines · Sean Christopherson

*** 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?

Yes, except for true runtime entries where a CPUID leaf is dynamic based on other CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.

Quoted 4 lines

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.

Quoted 5 lines

It is userspace's responsibility to provide a sane, correct setup. The one exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an unsupported virtual address width, the argument being that a malicious userspace could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a VMCS field.

Sorry could you elaborate an example of such attack? :)

Quoted 12 lines

The reason for KVM punting to userspace is that it's all but impossible to define what is/isn't sane. A really good example would be an alternative we (Google) considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly miss reserved bit #PFs.

Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in anticipation of eventual migration. So long as userspace doesn't actually enumerate memslots in the illegal address space, KVM would be able to treat such accesses as emulated MMIO, and would only need to intercept #PF(RSVD).

Quoted 3 lines

Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR definitely qualifies as insane since it really can't work correctly, but in our opinion it was far superior to running with allow_smaller_maxphyaddr=true.

I guess everyone wants performance.