KVM: VMX: Handle SEAMCALL or TDCALL VM-Exits
Subsystems (4)
Sean Christopherson
maintainerv2 cover KVM: VMX: Handle SEAMCALL or TDCALL VM-Exits
Add VM-Exit handlers for SEAMCALL and TDCALL as the instructions are gated only by VMXON, and so a buggy/misbehaving guest will likely be terminated (because KVM bails with KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON) if the guest executes SEAMCALL or TDCALL.
v2:
- Apply the behavior only to VMX. KVM can't inject #UDs for TDX, and the TDX-Module is supposed to handle SEAMCALL. [Chao]
- Fix nested exit handling (inverted return). [Xiaoyao]
- WARN if the TDX-Module punts a SEAMCALL exit to KVM. [Xiaoyao]
- Fix typos. [Binbin]
v1: https://lore.kernel.org/all/[email protected]
Sean Christopherson (2):
KVM: VMX: Inject #UD if guest tries to execute SEAMCALL or TDCALL
KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way out to KVM
arch/x86/include/uapi/asm/vmx.h | 1 + arch/x86/kvm/vmx/nested.c | 8 ++++++++ arch/x86/kvm/vmx/tdx.c | 3 +++ arch/x86/kvm/vmx/vmx.c | 8 ++++++++ 4 files changed, 20 insertions(+)
base-commit: 6b36119b94d0b2bb8cea9d512017efafd461d6ac
Signature · 1 line
2.51.0.858.gf9c4a03a3a-goog
Sean Christopherson
maintainerPATCH v2 1/2 KVM: VMX: Inject #UD if guest tries to execute SEAMCALL or TDCALL
Add VMX exit handlers for SEAMCALL and TDCALL to inject a #UD if a non-TD guest attempts to execute SEAMCALL or TDCALL. Neither SEAMCALL nor TDCALL is gated by any software enablement other than VMXON, and so will generate a VM-Exit instead of e.g. a native #UD when executed from the guest kernel.
Note! No unprivileged DoS of the L1 kernel is possible as TDCALL and SEAMCALL #GP at CPL > 0, and the CPL check is performed prior to the VMX non-root (VM-Exit) check, i.e. userspace can't crash the VM. And for a nested guest, KVM forwards unknown exits to L1, i.e. an L2 kernel can crash itself, but not L1.
Note #2! The Intel® Trust Domain CPU Architectural Extensions spec's pseudocode shows the CPL > 0 check for SEAMCALL coming _after_ the VM-Exit, but that appears to be a documentation bug (likely because the CPL > 0 check was incorrectly bundled with other lower-priority #GP checks). Testing on SPR and EMR shows that the CPL > 0 check is performed before the VMX non-root check, i.e. SEAMCALL #GPs when executed in usermode.
Note #3! The aforementioned Trust Domain spec uses confusing pseudocode that says that SEAMCALL will #UD if executed "inSEAM", but "inSEAM" specifically means in SEAM Root Mode, i.e. in the TDX-Module. The long- form description explicitly states that SEAMCALL generates an exit when executed in "SEAM VMX non-root operation". But that's a moot point as the TDX-Module injects #UD if the guest attempts to execute SEAMCALL, as documented in the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
--- arch/x86/include/uapi/asm/vmx.h | 1 + arch/x86/kvm/vmx/nested.c | 8 ++++++++ arch/x86/kvm/vmx/vmx.c | 8 ++++++++ 3 files changed, 17 insertions(+)
arch/x86/include/uapi/asm/vmx.h +1 -0
@@ -93,6 +93,7 @@#define EXIT_REASON_TPAUSE 68#define EXIT_REASON_BUS_LOCK 74#define EXIT_REASON_NOTIFY 75 +#define EXIT_REASON_SEAMCALL 76#define EXIT_REASON_TDCALL 77#define EXIT_REASON_MSR_READ_IMM 84#define EXIT_REASON_MSR_WRITE_IMM 85
arch/x86/kvm/vmx/nested.c +8 -0
@@ -6728,6 +6728,14 @@ static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu,case EXIT_REASON_NOTIFY :/* Notify VM exit is not exposed to L1 */ return false; +case EXIT_REASON_SEAMCALL : +case EXIT_REASON_TDCALL : +/* +* SEAMCALL and TDCALL unconditionally VM-Exit, but aren't +* virtualized by KVM for L1 hypervisors, i.e. L1 should +* never want or expect such an exit. +*/ +return false;default :return true; }
arch/x86/kvm/vmx/vmx.c +8 -0
@@ -6033,6 +6033,12 @@ static int handle_vmx_instruction(struct kvm_vcpu *vcpu)return 1 ; } +static int handle_tdx_instruction (struct kvm_vcpu *vcpu) +{ +kvm_queue_exception (vcpu,UD_VECTOR ); +return 1 ; +} +#ifndef CONFIG_X86_SGX_KVM static int handle_encls (struct kvm_vcpu *vcpu) {
@@ -6158,6 +6164,8 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = { [EXIT_REASON_ENCLS] = handle_encls, [EXIT_REASON_BUS_LOCK] = handle_bus_lock_vmexit, [EXIT_REASON_NOTIFY] = handle_notify, + [EXIT_REASON_SEAMCALL] = handle_tdx_instruction, + [EXIT_REASON_TDCALL] = handle_tdx_instruction, [EXIT_REASON_MSR_READ_IMM] = handle_rdmsr_imm, [EXIT_REASON_MSR_WRITE_IMM] = handle_wrmsr_imm, };
Signature · 1 line
2.51.0.858.gf9c4a03a3a-goog
Re: [PATCH v2 1/2] KVM: VMX: Inject #UD if guest tries to execute SEAMCALL or TDCALL
Quoted 27 lines · Sean Christopherson
Add VMX exit handlers for SEAMCALL and TDCALL to inject a #UD if a non-TD guest attempts to execute SEAMCALL or TDCALL. Neither SEAMCALL nor TDCALL is gated by any software enablement other than VMXON, and so will generate a VM-Exit instead of e.g. a native #UD when executed from the guest kernel.
Note! No unprivileged DoS of the L1 kernel is possible as TDCALL and SEAMCALL #GP at CPL > 0, and the CPL check is performed prior to the VMX non-root (VM-Exit) check, i.e. userspace can't crash the VM. And for a nested guest, KVM forwards unknown exits to L1, i.e. an L2 kernel can crash itself, but not L1.
Note #2! The Intel® Trust Domain CPU Architectural Extensions spec's pseudocode shows the CPL > 0 check for SEAMCALL coming _after_ the VM-Exit, but that appears to be a documentation bug (likely because the CPL > 0 check was incorrectly bundled with other lower-priority #GP checks). Testing on SPR and EMR shows that the CPL > 0 check is performed before the VMX non-root check, i.e. SEAMCALL #GPs when executed in usermode.
Note #3! The aforementioned Trust Domain spec uses confusing pseudocode that says that SEAMCALL will #UD if executed "inSEAM", but "inSEAM" specifically means in SEAM Root Mode, i.e. in the TDX-Module. The long- form description explicitly states that SEAMCALL generates an exit when executed in "SEAM VMX non-root operation". But that's a moot point as the TDX-Module injects #UD if the guest attempts to execute SEAMCALL, as documented in the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
Quoted 7 lines
Cc Kai Huang <[email protected]>Cc Xiaoyao Li <[email protected]>Cc Rick Edgecombe <[email protected]>Cc Dan Williams <[email protected]>Cc Binbin Wu <[email protected]>Signed-off-by Sean Christopherson <[email protected]>
Quoted 63 lines
--- arch/x86/include/uapi/asm/vmx.h | 1 + arch/x86/kvm/vmx/nested.c | 8 ++++++++ arch/x86/kvm/vmx/vmx.c | 8 ++++++++ 3 files changed, 17 insertions(+)arch/x86/include/uapi/asm/vmx.h +1
-0@@ -93,6 +93,7 @@#define EXIT_REASON_TPAUSE 68#define EXIT_REASON_BUS_LOCK 74#define EXIT_REASON_NOTIFY 75 +#define EXIT_REASON_SEAMCALL 76#define EXIT_REASON_TDCALL 77#define EXIT_REASON_MSR_READ_IMM 84#define EXIT_REASON_MSR_WRITE_IMM 85arch/x86/kvm/vmx/nested.c +8
-0@@ -6728,6 +6728,14 @@ static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu,case EXIT_REASON_NOTIFY :/* Notify VM exit is not exposed to L1 */ return false; +case EXIT_REASON_SEAMCALL : +case EXIT_REASON_TDCALL : +/* +* SEAMCALL and TDCALL unconditionally VM-Exit, but aren't +* virtualized by KVM for L1 hypervisors, i.e. L1 should +* never want or expect such an exit. +*/ +return false;default :return true; }arch/x86/kvm/vmx/vmx.c +8
-0@@ -6033,6 +6033,12 @@ static int handle_vmx_instruction(struct kvm_vcpu *vcpu)return 1 ; } +static int handle_tdx_instruction (struct kvm_vcpu *vcpu) +{ +kvm_queue_exception (vcpu,UD_VECTOR ); +return 1 ; +} +#ifndef CONFIG_X86_SGX_KVM static int handle_encls (struct kvm_vcpu *vcpu) {@@ -6158,6 +6164,8 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = { [EXIT_REASON_ENCLS] = handle_encls, [EXIT_REASON_BUS_LOCK] = handle_bus_lock_vmexit, [EXIT_REASON_NOTIFY] = handle_notify, + [EXIT_REASON_SEAMCALL] = handle_tdx_instruction, + [EXIT_REASON_TDCALL] = handle_tdx_instruction, [EXIT_REASON_MSR_READ_IMM] = handle_rdmsr_imm, [EXIT_REASON_MSR_WRITE_IMM] = handle_wrmsr_imm, };
Re: [PATCH v2 1/2] KVM: VMX: Inject #UD if guest tries to execute SEAMCALL or TDCALL
In reply to Sean ChristophersonQuoted 27 lines · Sean Christopherson
Add VMX exit handlers for SEAMCALL and TDCALL to inject a #UD if a non-TD guest attempts to execute SEAMCALL or TDCALL. Neither SEAMCALL nor TDCALL is gated by any software enablement other than VMXON, and so will generate a VM-Exit instead of e.g. a native #UD when executed from the guest kernel.
Note! No unprivileged DoS of the L1 kernel is possible as TDCALL and SEAMCALL #GP at CPL > 0, and the CPL check is performed prior to the VMX non-root (VM-Exit) check, i.e. userspace can't crash the VM. And for a nested guest, KVM forwards unknown exits to L1, i.e. an L2 kernel can crash itself, but not L1.
Note #2! The Intel® Trust Domain CPU Architectural Extensions spec's pseudocode shows the CPL > 0 check for SEAMCALL coming _after_ the VM-Exit, but that appears to be a documentation bug (likely because the CPL > 0 check was incorrectly bundled with other lower-priority #GP checks). Testing on SPR and EMR shows that the CPL > 0 check is performed before the VMX non-root check, i.e. SEAMCALL #GPs when executed in usermode.
Note #3! The aforementioned Trust Domain spec uses confusing pseudocode that says that SEAMCALL will #UD if executed "inSEAM", but "inSEAM" specifically means in SEAM Root Mode, i.e. in the TDX-Module. The long- form description explicitly states that SEAMCALL generates an exit when executed in "SEAM VMX non-root operation". But that's a moot point as the TDX-Module injects #UD if the guest attempts to execute SEAMCALL, as documented in the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
Quoted 7 lines
Cc Kai Huang <[email protected]>Cc Xiaoyao Li <[email protected]>Cc Rick Edgecombe <[email protected]>Cc Dan Williams <[email protected]>Cc Binbin Wu <[email protected]>Signed-off-by Sean Christopherson <[email protected]>
Re: [PATCH v2 1/2] KVM: VMX: Inject #UD if guest tries to execute SEAMCALL or TDCALL
In reply to Sean ChristophersonQuoted 5 lines
arch/x86/kvm/vmx/nested.c +0
-0@@ -6728,6 +6728,14 @@ static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu,case EXIT_REASON_NOTIFY :
arch/x86/kvm/vmx/nested.c static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu, · line 6738
Quoted 10 lines
arch/x86/kvm/vmx/nested.c +8
-0/* Notify VM exit is not exposed to L1 */ return false; +case EXIT_REASON_SEAMCALL : +case EXIT_REASON_TDCALL : +/* +* SEAMCALL and TDCALL unconditionally VM-Exit, but aren't +* virtualized by KVM for L1 hypervisors, i.e. L1 should +* never want or expect such an exit. +*/ +return false;
Sorry for commenting late.
I think from emulating hardware behaviour's perspective, if L1 doesn't support TDX (obviously true), SEAMCALL/TDCALL in L2 should cause VMEXIT to L1. In other words, L1 is expecting a VMEXIT in such case. Whether L1 can handle such VMEXIT is another story -- it may inject a #UD to L2 or may not (similar to the current upstream KVM), but it is L1's responsibility.
So I think while this patch certainly honors the correct behaviour for L2, it doesn't honor for L1. But I think ultimately L1 should be the one who is responsible for emulating hardware behaviour for L2.
E.g., assuming we have a KVM selftest in L1 to test SEAMCALL/TDCALL in normal VMX L2. L1 should be able to catch it's own bug when such VMEXIT isn't handled correctly. But with this patch, L1 will never be able to catch this IIUC.
Sean Christopherson
maintainerRe: [PATCH v2 1/2] KVM: VMX: Inject #UD if guest tries to execute SEAMCALL or TDCALL
Quoted 18 lines · Kai Huang
Quoted 5 lines
arch/x86/kvm/vmx/nested.c +0
-0@@ -6728,6 +6728,14 @@ static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu,case EXIT_REASON_NOTIFY :arch/x86/kvm/vmx/nested.c static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu, · line 6738
Quoted 10 lines
arch/x86/kvm/vmx/nested.c +8
-0/* Notify VM exit is not exposed to L1 */ return false; +case EXIT_REASON_SEAMCALL : +case EXIT_REASON_TDCALL : +/* +* SEAMCALL and TDCALL unconditionally VM-Exit, but aren't +* virtualized by KVM for L1 hypervisors, i.e. L1 should +* never want or expect such an exit. +*/ +return false;Sorry for commenting late.
Quoted 3 lines
I think from emulating hardware behaviour's perspective, if L1 doesn't support TDX (obviously true), SEAMCALL/TDCALL in L2 should cause VMEXIT to L1. In other words, L1 is expecting a VMEXIT in such case.
No, because from L1's perspective, the opcodes map to undefined instructions and thus should #UD in L2. There's no super explicit enumeration, but IMO it's fair to say that for L1 to think the instructions exists, it would need to observe IA32_SEAMRR_PHYS_{BASE,MASK} for SEAMCALL, and MSR_IA32_MKTME_KEYID_PARTITIONING as well for TDCALL. KVM doesn't emulate any of those instructions, and so L1 should never expect SEAMCALL or TDCALL to do anything other than #UD.
Quoted 12 lines
Whether L1 can handle such VMEXIT is another story -- it may inject a #UD to L2 or may not (similar to the current upstream KVM), but it is L1's responsibility.
So I think while this patch certainly honors the correct behaviour for L2, it doesn't honor for L1. But I think ultimately L1 should be the one who is responsible for emulating hardware behaviour for L2.
E.g., assuming we have a KVM selftest in L1 to test SEAMCALL/TDCALL in normal VMX L2. L1 should be able to catch it's own bug when such VMEXIT isn't handled correctly. But with this patch, L1 will never be able to catch this IIUC.
Re: [PATCH v2 1/2] KVM: VMX: Inject #UD if guest tries to execute SEAMCALL or TDCALL
Quoted 23 lines · Sean Christopherson
Quoted 18 lines · Kai Huang
Quoted 5 lines
arch/x86/kvm/vmx/nested.c +0
-0@@ -6728,6 +6728,14 @@ static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu,case EXIT_REASON_NOTIFY :arch/x86/kvm/vmx/nested.c static bool nested_vmx_l1_wants_exit(struct kvm_vcpu *vcpu, · line 6738
Quoted 10 lines
arch/x86/kvm/vmx/nested.c +8
-0/* Notify VM exit is not exposed to L1 */ return false; +case EXIT_REASON_SEAMCALL : +case EXIT_REASON_TDCALL : +/* +* SEAMCALL and TDCALL unconditionally VM-Exit, but aren't +* virtualized by KVM for L1 hypervisors, i.e. L1 should +* never want or expect such an exit. +*/ +return false;Sorry for commenting late.
Quoted 3 lines
I think from emulating hardware behaviour's perspective, if L1 doesn't support TDX (obviously true), SEAMCALL/TDCALL in L2 should cause VMEXIT to L1. In other words, L1 is expecting a VMEXIT in such case.
Quoted 7 lines
No, because from L1's perspective, the opcodes map to undefined instructions and thus should #UD in L2. There's no super explicit enumeration, but IMO it's fair to say that for L1 to think the instructions exists, it would need to observe IA32_SEAMRR_PHYS_{BASE,MASK} for SEAMCALL, and
MSR_IA32_MKTME_KEYID_PARTITIONINGas well for TDCALL. KVM doesn't emulate any of those instructions, and so L1 should never expect SEAMCALL or TDCALL to do anything other than #UD.
Oh right. I forgot the SEAMCALL/TDCALL VMEXIT only happens on TDX-capable machine. :-)
Quoted 1 line
Re: [PATCH v2 1/2] KVM: VMX: Inject #UD if guest tries to execute SEAMCALL or TDCALL
In reply to Sean ChristophersonQuoted 27 lines · Sean Christopherson
Add VMX exit handlers for SEAMCALL and TDCALL to inject a #UD if a non-TD guest attempts to execute SEAMCALL or TDCALL. Neither SEAMCALL nor TDCALL is gated by any software enablement other than VMXON, and so will generate a VM-Exit instead of e.g. a native #UD when executed from the guest kernel.
Note! No unprivileged DoS of the L1 kernel is possible as TDCALL and SEAMCALL #GP at CPL > 0, and the CPL check is performed prior to the VMX non-root (VM-Exit) check, i.e. userspace can't crash the VM. And for a nested guest, KVM forwards unknown exits to L1, i.e. an L2 kernel can crash itself, but not L1.
Note #2! The Intel® Trust Domain CPU Architectural Extensions spec's pseudocode shows the CPL > 0 check for SEAMCALL coming _after_ the VM-Exit, but that appears to be a documentation bug (likely because the CPL > 0 check was incorrectly bundled with other lower-priority #GP checks). Testing on SPR and EMR shows that the CPL > 0 check is performed before the VMX non-root check, i.e. SEAMCALL #GPs when executed in usermode.
Note #3! The aforementioned Trust Domain spec uses confusing pseudocode that says that SEAMCALL will #UD if executed "inSEAM", but "inSEAM" specifically means in SEAM Root Mode, i.e. in the TDX-Module. The long- form description explicitly states that SEAMCALL generates an exit when executed in "SEAM VMX non-root operation". But that's a moot point as the TDX-Module injects #UD if the guest attempts to execute SEAMCALL, as documented in the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
Quoted 8 lines
Cc Kai Huang <[email protected]>Cc Xiaoyao Li <[email protected]>Cc Rick Edgecombe <[email protected]>Cc Dan Williams <[email protected]>Cc Binbin Wu <[email protected]>Signed-off-by Sean Christopherson <[email protected]>
Sean Christopherson
maintainerPATCH v2 2/2 KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way out to KVM
In reply to Sean ChristophersonWARN if KVM observes a SEAMCALL VM-Exit while running a TD guest, as the TDX-Module is supposed to inject a #UD, per the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
--- arch/x86/kvm/vmx/tdx.c | 3 +++ 1 file changed, 3 insertions(+)
arch/x86/kvm/vmx/tdx.c +3 -0
@@ -2148,6 +2148,9 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)* - If it's not an MSMI, no need to do anything here. */ return 1 ; +case EXIT_REASON_SEAMCALL : +WARN_ON_ONCE (1 ); +break ;default :break ; }
Signature · 1 line
2.51.0.858.gf9c4a03a3a-goog
Re: [PATCH v2 2/2] KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way out to KVM
Quoted 6 lines · Sean Christopherson
WARN if KVM observes a SEAMCALL VM-Exit while running a TD guest, as the TDX-Module is supposed to inject a #UD, per the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
Reported-by Xiaoyao Li <[email protected]>Signed-off-by Sean Christopherson <[email protected]>
Quoted 18 lines
--- arch/x86/kvm/vmx/tdx.c | 3 +++ 1 file changed, 3 insertions(+)arch/x86/kvm/vmx/tdx.c +3
-0@@ -2148,6 +2148,9 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)* - If it's not an MSMI, no need to do anything here. */ return 1 ; +case EXIT_REASON_SEAMCALL : +WARN_ON_ONCE (1 ); +break ;default :break ; }
Re: [PATCH v2 2/2] KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way out to KVM
In reply to Sean ChristophersonQuoted 6 lines · Sean Christopherson
WARN if KVM observes a SEAMCALL VM-Exit while running a TD guest, as the TDX-Module is supposed to inject a #UD, per the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
Reported-by Xiaoyao Li <[email protected]>Signed-off-by Sean Christopherson <[email protected]>
Re: [PATCH v2 2/2] KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way out to KVM
In reply to Sean ChristophersonQuoted 15 lines · Sean Christopherson
WARN if KVM observes a SEAMCALL VM-Exit while running a TD guest, as the TDX-Module is supposed to inject a #UD, per the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
Reported-by Xiaoyao Li <[email protected]>Signed-off-by Sean Christopherson <[email protected]>--- arch/x86/kvm/vmx/tdx.c | 3 +++ 1 file changed, 3 insertions(+)arch/x86/kvm/vmx/tdx.c +0
-0
arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 2154
Quoted 7 lines
arch/x86/kvm/vmx/tdx.c +3
-0@@ -2148,6 +2148,9 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)* - If it's not an MSMI, no need to do anything here. */ return 1 ; +case EXIT_REASON_SEAMCALL : +WARN_ON_ONCE (1 ); +break ;
While this exit should never happen from a TDX guest, I am wondering why we need to explicitly handle the SEAMCALL? E.g., per "Unconditionally Blocked Instructions" ENCLS/ENCLV are also listed, therefore EXIT_REASON_ELCLS/ENCLV should never come from a TDX guest either.
Sean Christopherson
maintainerRe: [PATCH v2 2/2] KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way out to KVM
Quoted 20 lines · Kai Huang
Quoted 19 lines · Sean Christopherson
WARN if KVM observes a SEAMCALL VM-Exit while running a TD guest, as the TDX-Module is supposed to inject a #UD, per the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
Reported-by Xiaoyao Li <[email protected]>Signed-off-by Sean Christopherson <[email protected]>--- arch/x86/kvm/vmx/tdx.c | 3 +++ 1 file changed, 3 insertions(+)arch/x86/kvm/vmx/tdx.c +1
-0@@ -2148,6 +2148,9 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)* - If it's not an MSMI, no need to do anything here. */ return 1 ; +case EXIT_REASON_SEAMCALL :
Quoted 8 lines
arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 2154
Quoted 3 lines
arch/x86/kvm/vmx/tdx.c +2
-0+WARN_ON_ONCE (1 ); +break ;While this exit should never happen from a TDX guest, I am wondering why we need to explicitly handle the SEAMCALL? E.g., per "Unconditionally Blocked Instructions" ENCLS/ENCLV are also listed, therefore EXIT_REASON_ELCLS/ENCLV should never come from a TDX guest either.
Good point. SEAMCALL was obviously top of mind, I didn't think about all the other exits that should be impossible.
I haven't looked closely, at all, but I wonder if we can get away with this?
arch/x86/kvm/vmx/tdx.c +2 -0
@@ -2149,6 +2149,8 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)*/ return 1 ;default : +/* All other known exits should be handled by the TDX-Module. */ +WARN_ON_ONCE (exit_reason.basic <=EXIT_REASON_TDCALL );break ; }
Re: [PATCH v2 2/2] KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way out to KVM
Quoted 40 lines · Sean Christopherson
Quoted 20 lines · Kai Huang
Quoted 19 lines · Sean Christopherson
WARN if KVM observes a SEAMCALL VM-Exit while running a TD guest, as the TDX-Module is supposed to inject a #UD, per the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
Reported-by Xiaoyao Li <[email protected]>Signed-off-by Sean Christopherson <[email protected]>--- arch/x86/kvm/vmx/tdx.c | 3 +++ 1 file changed, 3 insertions(+)arch/x86/kvm/vmx/tdx.c +1
-0@@ -2148,6 +2148,9 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)* - If it's not an MSMI, no need to do anything here. */ return 1 ; +case EXIT_REASON_SEAMCALL :Quoted 8 lines
arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 2154
Quoted 3 lines
arch/x86/kvm/vmx/tdx.c +2
-0+WARN_ON_ONCE (1 ); +break ;While this exit should never happen from a TDX guest, I am wondering why we need to explicitly handle the SEAMCALL? E.g., per "Unconditionally Blocked Instructions" ENCLS/ENCLV are also listed, therefore EXIT_REASON_ELCLS/ENCLV should never come from a TDX guest either.
Good point. SEAMCALL was obviously top of mind, I didn't think about all the other exits that should be impossible.
I haven't looked closely, at all, but I wonder if we can get away with this?
arch/x86/kvm/vmx/tdx.c +0
-0
arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 2155
Quoted 7 lines
arch/x86/kvm/vmx/tdx.c +2
-0@@ -2149,6 +2149,8 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)*/ return 1 ;default : +/* All other known exits should be handled by the TDX-Module. */ +WARN_ON_ONCE (exit_reason.basic <= c);break ; }
Not 100% sure, but should be fine? Needs more second eyes here.
E.g., when a new module feature makes another exit reason possible then presumably we need explicit opt-in to that feature.
Don't quite follow 'exit_reason.basic <= c' part, though. Maybe we can just unconditional WARN_ON_ONCE()?
Or we can do things similar to VMX:
vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n",
exit_reason.full);Or just get rid of this patch :-)
Re: [PATCH v2 2/2] KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way out to KVM
Quoted 36 lines · Kai Huang
Quoted 35 lines · Sean Christopherson
Quoted 20 lines · Kai Huang
Quoted 19 lines · Sean Christopherson
WARN if KVM observes a SEAMCALL VM-Exit while running a TD guest, as the TDX-Module is supposed to inject a #UD, per the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
Reported-by Xiaoyao Li <[email protected]>Signed-off-by Sean Christopherson <[email protected]>--- arch/x86/kvm/vmx/tdx.c | 3 +++ 1 file changed, 3 insertions(+)arch/x86/kvm/vmx/tdx.c +1
-0@@ -2148,6 +2148,9 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)* - If it's not an MSMI, no need to do anything here. */ return 1 ; +case EXIT_REASON_SEAMCALL :Quoted 8 lines
arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 2154
Quoted 3 lines
arch/x86/kvm/vmx/tdx.c +2
-0+WARN_ON_ONCE (1 ); +break ;While this exit should never happen from a TDX guest, I am wondering why we need to explicitly handle the SEAMCALL? E.g., per "Unconditionally Blocked Instructions" ENCLS/ENCLV are also listed, therefore EXIT_REASON_ELCLS/ENCLV should never come from a TDX guest either.
Good point. SEAMCALL was obviously top of mind, I didn't think about all the other exits that should be impossible.
I haven't looked closely, at all, but I wonder if we can get away with this?
Quoted 10 lines
arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 2153
Quoted 10 lines
arch/x86/kvm/vmx/tdx.c +2
-0@@ -2149,6 +2149,8 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)*/ return 1 ;default : +/* All other known exits should be handled by the TDX-Module. */ +WARN_ON_ONCE (exit_reason.basic <= c);
Sorry somehow I clobbered the text (at Saturday morning):
WARN_ON_ONCE(exit_reason.basic <= EXIT_REASON_TDCALL);
Quoted 10 lines
arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 2155
Quoted 2 lines
arch/x86/kvm/vmx/tdx.c +0
-0break ; }Not 100% sure, but should be fine? Needs more second eyes here.
E.g., when a new module feature makes another exit reason possible then presumably we need explicit opt-in to that feature.
Don't quite follow 'exit_reason.basic <= c' part, though. Maybe we can just unconditional
WARN_ON_ONCE()?
... and copied here..
Re: [PATCH v2 2/2] KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way out to KVM
In reply to Huang, KaiQuoted 57 lines · Kai
Quoted 40 lines · Sean Christopherson
Quoted 20 lines · Kai Huang
Quoted 19 lines · Sean Christopherson
WARN if KVM observes a SEAMCALL VM-Exit while running a TD guest, as the TDX-Module is supposed to inject a #UD, per the "Unconditionally Blocked Instructions" section of the TDX-Module base specification.
Reported-by Xiaoyao Li <[email protected]>Signed-off-by Sean Christopherson <[email protected]>--- arch/x86/kvm/vmx/tdx.c | 3 +++ 1 file changed, 3 insertions(+)arch/x86/kvm/vmx/tdx.c +1
-0@@ -2148,6 +2148,9 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)* - If it's not an MSMI, no need to do anything here. */ return 1 ; +case EXIT_REASON_SEAMCALL :Quoted 8 lines
arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 2154
Quoted 3 lines
arch/x86/kvm/vmx/tdx.c +2
-0+WARN_ON_ONCE (1 ); +break ;While this exit should never happen from a TDX guest, I am wondering why we need to explicitly handle the SEAMCALL? E.g., per "Unconditionally Blocked Instructions" ENCLS/ENCLV are also listed, therefore EXIT_REASON_ELCLS/ENCLV should never come from a TDX guest either.
Good point. SEAMCALL was obviously top of mind, I didn't think about all the other exits that should be impossible.
I haven't looked closely, at all, but I wonder if we can get away with this?
arch/x86/kvm/vmx/tdx.c +0
-0arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 2155
Quoted 7 lines
arch/x86/kvm/vmx/tdx.c +2
-0@@ -2149,6 +2149,8 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)*/ return 1 ;default : +/* All other known exits should be handled by the TDX-Module. */ +WARN_ON_ONCE (exit_reason.basic <= c);break ; }Not 100% sure, but should be fine? Needs more second eyes here.
E.g., when a new module feature makes another exit reason possible then presumably we need explicit opt-in to that feature.
Don't quite follow 'exit_reason.basic <= c' part, though. Maybe we can just unconditional
WARN_ON_ONCE()?
Quoted 6 lines
Or we can do things similar to VMX:
vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason.full);Or just get rid of this patch :-)
I agree to it.
WARN_ON_ONCE() seems not provide more information except that we can identify quickly there is TDX module bug when it gets hit.
But it's not too hard to get these information from the KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON with vp_enter_ret in data[0].
And if we add WARN_ON_ONCE() here, we need to evaluate if it needs to update "EXIT_REASON_TDCALL" everytime a new EXIT reason is introduced. e.g., currently the largest exit reason number defined in SDM is 79 (for WRMSRLIST) and it is actually handled by TDX module and KVM cannot receive it from a TD vcpu.
Sean Christopherson
maintainerRe: [PATCH v2 2/2] KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way out to KVM
Quoted 39 lines · Xiaoyao Li
Quoted 38 lines · Kai
Quoted 16 lines · Sean Christopherson
Quoted 4 lines · Kai Huang
While this exit should never happen from a TDX guest, I am wondering why we need to explicitly handle the SEAMCALL? E.g., per "Unconditionally Blocked Instructions" ENCLS/ENCLV are also listed, therefore EXIT_REASON_ELCLS/ENCLV should never come from a TDX guest either.
Good point. SEAMCALL was obviously top of mind, I didn't think about all the other exits that should be impossible.
I haven't looked closely, at all, but I wonder if we can get away with this?
arch/x86/kvm/vmx/tdx.c +0
-0arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 2155
Quoted 7 lines
arch/x86/kvm/vmx/tdx.c +2
-0@@ -2149,6 +2149,8 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)*/ return 1 ;default : +/* All other known exits should be handled by the TDX-Module. */ +WARN_ON_ONCE (exit_reason.basic <= c);break ; }Not 100% sure, but should be fine? Needs more second eyes here.
E.g., when a new module feature makes another exit reason possible then presumably we need explicit opt-in to that feature.
Don't quite follow 'exit_reason.basic <= c' part, though. Maybe we can just unconditional
WARN_ON_ONCE()?Or we can do things similar to VMX:
vcpu_unimpl(vcpu, "vmx: unexpected exit reason 0x%x\n", exit_reason.full);
Quoted 6 lines
Quoted 1 line
Or just get rid of this patch :-)
I agree to it.
WARN_ON_ONCE()seems not provide more information except that we can identify quickly there is TDX module bug when it gets hit.
WARNs are helpful for fuzzing, e.g. with syzkaller, where userspace isn't going to complain or even log anything if KVM_RUN fails.
Quoted 8 lines
But it's not too hard to get these information from the
KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASONwithvp_enter_retin data[0].And if we add
WARN_ON_ONCE()here, we need to evaluate if it needs to update "EXIT_REASON_TDCALL" everytime a new EXIT reason is introduced. e.g., currently the largest exit reason number defined in SDM is 79 (for WRMSRLIST) and it is actually handled by TDX module and KVM cannot receive it from a TD vcpu.
Hmm, to some extent this is a case of "don't let perfect be the enemy of good enough". Just because we can't WARN on _all_ unexpected exits doesn't make WARNing on the others useless.
That said, I agree that making the WARN conditional on the exit reason would be a maintenance burden. And I also agree with the implied "rule" that TDX should follow whatever VMX is doing (and ideally SVM would behavior identically as well).
At the very least, we should consolidate that code. And then we can have a broader discussion on how exactly to add a WARN. E.g. if for some reason we can't WARN unconditionally, we could add an off-by-default module param.
I'll drop this patch and post a mini-series with the below to start a general conversation on whether or not to WARN on unexpected exits.
arch/x86/include/asm/kvm_host.h +1 -0
@@ -2167,6 +2167,7 @@ void __kvm_prepare_emulation_failure_exit(struct kvm_vcpu *vcpu,void kvm_prepare_emulation_failure_exit (struct kvm_vcpu *vcpu);void kvm_prepare_event_vectoring_exit (struct kvm_vcpu *vcpu,gpa_t gpa); +void kvm_prepare_unexpected_reason_exit (struct kvm_vcpu *vcpu,u64 exit_reason);void kvm_enable_efer_bits (u64 );bool kvm_valid_efer (struct kvm_vcpu *vcpu,u64 efer);
arch/x86/kvm/svm/svm.c +1 -6
@@ -3451,13 +3451,8 @@ static bool svm_check_exit_valid(u64 exit_code)static int svm_handle_invalid_exit (struct kvm_vcpu *vcpu,u64 exit_code) { -vcpu_unimpl (vcpu,"svm: unexpected exit reason 0x%llx\n" , exit_code);dump_vmcb (vcpu); - vcpu->run->exit_reason =KVM_EXIT_INTERNAL_ERROR ; - vcpu->run->internal.suberror =KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON ; - vcpu->run->internal.ndata =2 ; - vcpu->run->internal.data[0 ] = exit_code; - vcpu->run->internal.data[1 ] = vcpu->arch.last_vmentry_cpu; +kvm_prepare_unexpected_reason_exit (vcpu, exit_code);return 0 ; }
arch/x86/kvm/vmx/tdx.c +1 -5
@@ -2145,11 +2145,7 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) } unhandled_exit: - vcpu->run->exit_reason =KVM_EXIT_INTERNAL_ERROR ; - vcpu->run->internal.suberror =KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON ; - vcpu->run->internal.ndata =2 ; - vcpu->run->internal.data[0 ] = vp_enter_ret; - vcpu->run->internal.data[1 ] = vcpu->arch.last_vmentry_cpu; +kvm_prepare_unexpected_reason_exit (vcpu, vp_enter_ret);return 0 ; }
arch/x86/kvm/vmx/vmx.c +1 -8
@@ -6642,15 +6642,8 @@ static int __vmx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)return kvm_vmx_exit_handlers[exit_handler_index](vcpu); unexpected_vmexit: -vcpu_unimpl (vcpu,"vmx: unexpected exit reason 0x%x\n" , - exit_reason.full);dump_vmcs (vcpu); - vcpu->run->exit_reason =KVM_EXIT_INTERNAL_ERROR ; - vcpu->run->internal.suberror = -KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON ; - vcpu->run->internal.ndata =2 ; - vcpu->run->internal.data[0 ] = exit_reason.full; - vcpu->run->internal.data[1 ] = vcpu->arch.last_vmentry_cpu; +kvm_prepare_unexpected_reason_exit (vcpu, exit_reason.full);return 0 ; }
arch/x86/kvm/x86.c +12 -0
@@ -9110,6 +9110,18 @@ void kvm_prepare_event_vectoring_exit(struct kvm_vcpu *vcpu, gpa_t gpa) }EXPORT_SYMBOL_FOR_KVM_INTERNAL (kvm_prepare_event_vectoring_exit); +void kvm_prepare_unexpected_reason_exit (struct kvm_vcpu *vcpu,u64 exit_reason) +{ +vcpu_unimpl (vcpu,"unexpected exit reason 0x%llx\n" , exit_reason); + + vcpu->run->exit_reason =KVM_EXIT_INTERNAL_ERROR ; + vcpu->run->internal.suberror =KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON ; + vcpu->run->internal.ndata =2 ; + vcpu->run->internal.data[0 ] = exit_reason; + vcpu->run->internal.data[1 ] = vcpu->arch.last_vmentry_cpu; +} +EXPORT_SYMBOL_FOR_KVM_INTERNAL (kvm_prepare_unexpected_reason_exit); +static int handle_emulation_failure (struct kvm_vcpu *vcpu,int emulation_type) {struct kvm *kvm = vcpu->kvm;
Quoted 5 lines · Sean Christopherson
Add VM-Exit handlers for SEAMCALL and TDCALL as the instructions are gated only by VMXON, and so a buggy/misbehaving guest will likely be terminated (because KVM bails with
KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON) if the guest executes SEAMCALL or TDCALL.
Quoted 8 lines
v2:
- Apply the behavior only to VMX. KVM can't inject #UDs for TDX, and the TDX-Module is supposed to handle SEAMCALL. [Chao]
- Fix nested exit handling (inverted return). [Xiaoyao]
- WARN if the TDX-Module punts a SEAMCALL exit to KVM. [Xiaoyao]
- Fix typos. [Binbin][...]
Applied patch 1 to kvm-x86 fixes.
[1/2] KVM: VMX: Inject #UD if guest tries to execute SEAMCALL or TDCALL
https://github.com/kvm-x86/linux/commit/9d7dfb95da2c