| 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
| ||
|
Message-ID: <aPZPQCFNn__Vzz3O@google.com>
Date: Mon, 20 Oct 2025 08:03:28 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Xiaoyao Li <xiaoyao.li@...el.com>
Cc: Kai Huang <kai.huang@...el.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"pbonzini@...hat.com" <pbonzini@...hat.com>, Dan J Williams <dan.j.williams@...el.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Rick P Edgecombe <rick.p.edgecombe@...el.com>,
"binbin.wu@...ux.intel.com" <binbin.wu@...ux.intel.com>
Subject: Re: [PATCH v2 2/2] KVM: TDX: WARN if a SEAMCALL VM-Exit makes its way
out to KVM
On Mon, Oct 20, 2025, Xiaoyao Li wrote:
> On 10/18/2025 4:58 AM, Huang, Kai wrote:
> > On Fri, 2025-10-17 at 10:25 -0700, Sean Christopherson wrote:
> > > On Fri, Oct 17, 2025, Kai Huang wrote:
> > > > 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?
> > >
> > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > > index 097304bf1e1d..4c68444bd673 100644
> > > --- a/arch/x86/kvm/vmx/tdx.c
> > > +++ b/arch/x86/kvm/vmx/tdx.c
> > > @@ -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 :-)
>
> 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.
> 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.
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.
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 48598d017d6f..4fbe4b7ce1da 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -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);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index f14709a511aa..83e0d4d5f4c5 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -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;
}
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 326db9b9c567..079d9f13eddb 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -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;
}
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1021d3b65ea0..08f7957ed4c3 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -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;
}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b4b5d2d09634..c826cd05228a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -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;
Powered by blists - more mailing lists