linux-kernel - Re: [PATCH 11/16] KVM: TDX: Always block INIT/SIPI

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z36OYfRW9oPjW8be@google.com>
Date: Wed, 8 Jan 2025 06:40:33 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Binbin Wu <binbin.wu@...ux.intel.com>
Cc: Xiaoyao Li <xiaoyao.li@...el.com>, pbonzini@...hat.com, kvm@...r.kernel.org, 
	rick.p.edgecombe@...el.com, kai.huang@...el.com, adrian.hunter@...el.com, 
	reinette.chatre@...el.com, tony.lindgren@...ux.intel.com, 
	isaku.yamahata@...el.com, yan.y.zhao@...el.com, chao.gao@...el.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 11/16] KVM: TDX: Always block INIT/SIPI

On Wed, Jan 08, 2025, Binbin Wu wrote:
> On 1/8/2025 3:21 PM, Xiaoyao Li wrote:
> > On 12/9/2024 9:07 AM, Binbin Wu wrote:

...

> > > ---
> > >   arch/x86/kvm/lapic.c    |  2 +-
> > >   arch/x86/kvm/vmx/main.c | 19 ++++++++++++++++++-
> > >   2 files changed, 19 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 474e0a7c1069..f93c382344ee 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -3365,7 +3365,7 @@ int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
> > >         if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
> > >           kvm_vcpu_reset(vcpu, true);
> > > -        if (kvm_vcpu_is_bsp(apic->vcpu))
> > > +        if (kvm_vcpu_is_bsp(vcpu))
> > >               vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
> > >           else
> > >               vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
> > > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> > > index 8ec96646faec..7f933f821188 100644
> > > --- a/arch/x86/kvm/vmx/main.c
> > > +++ b/arch/x86/kvm/vmx/main.c
> > > @@ -115,6 +115,11 @@ static void vt_vcpu_free(struct kvm_vcpu *vcpu)
> > >     static void vt_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> > >   {
> > > +    /*
> > > +     * TDX has its own sequence to do init during TD build time (by
> > > +     * KVM_TDX_INIT_VCPU) and it doesn't support INIT event during TD
> > > +     * runtime.
> > > +     */
> > 
> > The first half is confusing. It seems to mix up init(ialization) with INIT
> > event.
> > 
> > And this callback is about *reset*, which can be due to INIT event or not.
> > That's why it has a second parameter of init_event. The comment needs to
> > clarify why reset is not needed for both cases.
> > 
> > I think we can just say TDX doesn't support vcpu reset no matter due to
> > INIT event or not.

That's not entirely accurate either though.  TDX does support KVM's version of
RESET, because KVM's RESET is "power-on", i.e. vCPU creation.  Emulation of
runtime RESET is userspace's responsibility.

The real reason why KVM doesn't do anything during KVM's RESET is that what
little setup KVM does/can do needs to be defered until after guest CPUID is
configured.

KVM should also WARN if a TDX vCPU gets INIT, no?

Side topic, the comment about x2APIC in tdx_vcpu_init() is too specific, e.g.
calling out that x2APIC support is enumerated in CPUID.0x1.ECX isn't necessary,
and stating that userspace must use KVM_SET_CPUID2 is flat out wrong.  Very
technically, KVM_SET_CPUID is also a valid option, it's just not used in practice
because it doesn't support setting non-zero indices (but in theory it could be
used to enable x2APIC).

E.g. something like this?

diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index d2e78e6675b9..e36fba94fa14 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -115,13 +115,10 @@ static void vt_vcpu_free(struct kvm_vcpu *vcpu)
 
 static void vt_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
 {
-       /*
-        * TDX has its own sequence to do init during TD build time (by
-        * KVM_TDX_INIT_VCPU) and it doesn't support INIT event during TD
-        * runtime.
-        */
-       if (is_td_vcpu(vcpu))
+       if (is_td_vcpu(vcpu)) {
+               tdx_vcpu_reset(vcpu, init_event);
                return;
+       }
 
        vmx_vcpu_reset(vcpu, init_event);
 }
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 9e490fccf073..a587f59167a7 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -2806,9 +2806,8 @@ static int tdx_vcpu_init(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
                return -EINVAL;
 
        /*
-        * As TDX requires X2APIC, set local apic mode to X2APIC.  User space
-        * VMM, e.g. qemu, is required to set CPUID[0x1].ecx.X2APIC=1 by
-        * KVM_SET_CPUID2.  Otherwise kvm_apic_set_base() will fail.
+        * TDX requires x2APIC, userspace is responsible for configuring guest
+        * CPUID accordingly.
         */
        apic_base = APIC_DEFAULT_PHYS_BASE | LAPIC_MODE_X2APIC |
                (kvm_vcpu_is_reset_bsp(vcpu) ? MSR_IA32_APICBASE_BSP : 0);
@@ -2827,6 +2826,19 @@ static int tdx_vcpu_init(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
        return 0;
 }
 
+void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
+{
+       /*
+        * Yell on INIT, as TDX doesn't support INIT, i.e. KVM should drop all
+        * INIT events.
+        *
+        * Defer initializing vCPU for RESET state until KVM_TDX_INIT_VCPU, as
+        * userspace needs to define the vCPU model before KVM can initialize
+        * vCPU state, e.g. to enable x2APIC.
+        */
+       WARN_ON_ONCE(init_event);
+}
+
 struct tdx_gmem_post_populate_arg {
        struct kvm_vcpu *vcpu;
        __u32 flags;