Re: [PATCH v2 2/3] KVM: TDX: Fix the exit reason handling
Xiaoyao Li <[email protected]>
| Newsgroups | dev.linux.lists.sashiko-reviews,org.kernel.vger.kvm |
|---|---|
| Message-ID | <[email protected]> |
On 8/13/2026 8:20 AM, Sean Christopherson wrote: > On Tue, Aug 11, 2026, Xiaoyao Li wrote: >> On 8/11/2026 7:57 AM, Sean Christopherson wrote: >>> On Mon, Aug 10, 2026, Xiaoyao Li wrote: >>>> On 8/10/2026 7:39 PM, [email protected] wrote: >>>>>> @@ -2147,7 +2156,7 @@ void tdx_get_exit_info(struct kvm_vcpu *vcpu, u32 *reason, >>>>>> struct vcpu_tdx *tdx = to_tdx(vcpu); >>>>>> *reason = tdx->vt.exit_reason.full; >>>>>> - if (*reason != -1u) { >>>>>> + if (tdx->vt.exit_reason.basic != -1) { >>>>> [Severity: Medium] >>>>> Will this check always evaluate to true due to C integer promotion rules? >>>>> >>>>> The basic field in union vmx_exit_reason is a 16-bit unsigned bitfield. When >>>>> comparing it to -1, the unsigned 16-bit value is promoted to a signed 32-bit >>>>> integer. If the value was set to -1 (65535), the comparison evaluates as >>>>> 65535 != -1, which is always true. >>>>> >>>> >>>> Well, how about something below on top of this patch? >>> >>> No, we need to not rely on magic exit_reason.basic values. Can't this be? >>> >>> if ((tdx->vp_enter_ret & TDX_SW_ERROR) != TDX_SW_ERROR) { >> >> I think the reason of checking if (*reason != -1u) in the original commit >> 095b71a03f49 ("KVM: TDX: Add a place holder to handle TDX VM exit") was >> that, -1u means there is no valid Exit Reason in the lower 32 bits in >> vp_enter_ret. >> >> Change it to check TDX_SW_ERROR, doesn't look correct to me. Set the >> EPT_MISCONFIG magic handling aside, TDX_SW_ERROR only means the SEAMCALL >> instruction faults, e.g., hitting #UD, #GP, or VMFAILINVALID. Just a small >> subset of the cases where there is no valid Exit Reason. > > Surely we can enumerate those cases? I.e. why can't that be something like: > > if (tdx_is_exit_reason_valid(vcpu)) > > where tdx_is_exit_reason_valid() decodes vp_enter_ret. Clobbering the entire > exit reason and then _relying_ on that clobbered state is gross and brittle. This a good suggestion. I implemented it as: Author: Xiaoyao Li <[email protected]> Date: Thu Aug 13 09:42:15 2026 +0800 KVM: TDX: Check if there is valid exit info based on vp_enter_ret Check if there is valid exit info based on vp_enter_ret instead of relying on the clobbered Exit Reason, in tdx_get_exit_info(). Current KVM uses "Exit Reason is not equal to the synthesized invalid Exit Reason, -1u," as the condition to identify there is a real TD Exit and valid exit infos. However, there is one issue with this approach: KVM updates the Exit Reason to the synthesized invalid Exit Reason for real EPT MISCONFIG as well. This is a false positive that real EPT MISCONFIG has valid exit infos. Though the issue can be addressed by changing the handling for real EPT MISCONFIG to not update the Exit Reason to the synthesized one, relying on the clobbered Exit Reason itself is brittle. Instead, check vp_enter_ret directly to identify if it is a valid Exit Reason. Fixes: da407fe45908 ("KVM: TDX: Handle EPT violation/misconfig exit") Cc: [email protected] Suggested-by: Sean Christopherson <[email protected]> Signed-off-by: Xiaoyao Li <[email protected]> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c index 7338ac0af693..f9036c0a8f9c 100644 --- a/arch/x86/kvm/vmx/tdx.c +++ b/arch/x86/kvm/vmx/tdx.c @@ -921,21 +921,27 @@ static __always_inline u32 tdcall_to_vmx_exit_reason(struct kvm_vcpu *vcpu) return EXIT_REASON_TDCALL; } -static __always_inline u32 tdx_to_vmx_exit_reason(struct kvm_vcpu *vcpu) +static __always_inline bool tdx_is_exit_reason_valid(u64 vp_enter_ret) { - struct vcpu_tdx *tdx = to_tdx(vcpu); - u32 exit_reason; - - switch (tdx->vp_enter_ret & TDX_SEAMCALL_STATUS_MASK) { + switch (vp_enter_ret & TDX_SEAMCALL_STATUS_MASK) { case TDX_SUCCESS: case TDX_NON_RECOVERABLE_VCPU: case TDX_NON_RECOVERABLE_TD: case TDX_NON_RECOVERABLE_TD_NON_ACCESSIBLE: case TDX_NON_RECOVERABLE_TD_WRONG_APIC_MODE: - break; + return true; default: - return -1u; + return false; } +} + +static __always_inline u32 tdx_to_vmx_exit_reason(struct kvm_vcpu *vcpu) +{ + struct vcpu_tdx *tdx = to_tdx(vcpu); + u32 exit_reason; + + if (!tdx_is_exit_reason_valid(tdx->vp_enter_ret)) + return -1u; exit_reason = tdx->vp_enter_ret; @@ -2144,7 +2150,7 @@ void tdx_get_exit_info(struct kvm_vcpu *vcpu, u32 *reason, struct vcpu_tdx *tdx = to_tdx(vcpu); *reason = tdx->vt.exit_reason.full; - if (*reason != -1u) { + if (tdx_is_exit_reason_valid(tdx->vp_enter_ret)) { *info1 = vmx_get_exit_qual(vcpu); *info2 = tdx->ext_exit_qualification; *intr_info = vmx_get_intr_info(vcpu);