| 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: <aQmmBadeFp/7CDmH@yzhao56-desk.sh.intel.com>
Date: Tue, 4 Nov 2025 15:06:45 +0800
From: Yan Zhao <yan.y.zhao@...el.com>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
<pbonzini@...hat.com>, "Kirill A. Shutemov" <kas@...nel.org>,
<kvm@...r.kernel.org>, <x86@...nel.org>, <linux-coco@...ts.linux.dev>,
<linux-kernel@...r.kernel.org>, Xiaoyao Li <xiaoyao.li@...el.com>, "Rick
Edgecombe" <rick.p.edgecombe@...el.com>, Hou Wenlong
<houwenlong.hwl@...group.com>
Subject: Re: [PATCH v5 1/4] KVM: TDX: Explicitly set user-return MSRs that
*may* be clobbered by the TDX-Module
Another nit:
Remove the tdx_user_return_msr_update_cache() in the comment of __tdx_bringup().
Or could we just invoke tdx_user_return_msr_update_cache() in
tdx_prepare_switch_to_guest()?
On Mon, Nov 03, 2025 at 02:20:18PM +0800, Yan Zhao wrote:
> On Thu, Oct 30, 2025 at 12:15:25PM -0700, Sean Christopherson wrote:
> > Set all user-return MSRs to their post-TD-exit value when preparing to run
> > a TDX vCPU to ensure the value that KVM expects to be loaded after running
> > the vCPU is indeed the value that's loaded in hardware. If the TDX-Module
> > doesn't actually enter the guest, i.e. doesn't do VM-Enter, then it won't
> > "restore" VMM state, i.e. won't clobber user-return MSRs to their expected
> > post-run values, in which case simply updating KVM's "cached" value will
> > effectively corrupt the cache due to hardware still holding the original
> > value.
> This paragraph is confusing.
>
> The flow for the TDX module for the user-return MSRs is:
>
> 1. Before entering guest, i.e., inside tdh_vp_enter(),
> a) if VM-Enter is guaranteed to succeed, load MSRs with saved guest value;
> b) otherwise, do nothing and return to VMM.
>
> 2. After VMExit, before returning to VMM,
> save guest value and restore MSRs to default values.
>
>
> Failure of tdh_vp_enter() (i.e., in case of 1.b), the hardware values of the
> MSRs should be either host value or default value, while with
> msrs->values[slot].curr being default value.
>
> As a result, the reasoning of "hardware still holding the original value" is not
> convincing, since the original value is exactly the host value.
>
> > In theory, KVM could conditionally update the current user-return value if
> > and only if tdh_vp_enter() succeeds, but in practice "success" doesn't
> > guarantee the TDX-Module actually entered the guest, e.g. if the TDX-Module
> > synthesizes an EPT Violation because it suspects a zero-step attack.
> >
> > Force-load the expected values instead of trying to decipher whether or
> > not the TDX-Module restored/clobbered MSRs, as the risk doesn't justify
> > the benefits. Effectively avoiding four WRMSRs once per run loop (even if
> > the vCPU is scheduled out, user-return MSRs only need to be reloaded if
> > the CPU exits to userspace or runs a non-TDX vCPU) is likely in the noise
> > when amortized over all entries, given the cost of running a TDX vCPU.
> > E.g. the cost of the WRMSRs is somewhere between ~300 and ~500 cycles,
> > whereas the cost of a _single_ roundtrip to/from a TDX guest is thousands
> > of cycles.
> >
> > Fixes: e0b4f31a3c65 ("KVM: TDX: restore user ret MSRs")
> > Cc: stable@...r.kernel.org
> > Cc: Yan Zhao <yan.y.zhao@...el.com>
> > Cc: Xiaoyao Li <xiaoyao.li@...el.com>
> > Cc: Rick Edgecombe <rick.p.edgecombe@...el.com>
> > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> > ---
> > arch/x86/include/asm/kvm_host.h | 1 -
> > arch/x86/kvm/vmx/tdx.c | 52 +++++++++++++++------------------
> > arch/x86/kvm/vmx/tdx.h | 1 -
> > arch/x86/kvm/x86.c | 9 ------
> > 4 files changed, 23 insertions(+), 40 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 48598d017d6f..d158dfd1842e 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -2378,7 +2378,6 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > int kvm_add_user_return_msr(u32 msr);
> > int kvm_find_user_return_msr(u32 msr);
> > int kvm_set_user_return_msr(unsigned index, u64 val, u64 mask);
> > -void kvm_user_return_msr_update_cache(unsigned int index, u64 val);
> > u64 kvm_get_user_return_msr(unsigned int slot);
> >
> > static inline bool kvm_is_supported_user_return_msr(u32 msr)
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 326db9b9c567..cde91a995076 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -763,25 +763,6 @@ static bool tdx_protected_apic_has_interrupt(struct kvm_vcpu *vcpu)
> > return tdx_vcpu_state_details_intr_pending(vcpu_state_details);
> > }
> >
> > -/*
> > - * Compared to vmx_prepare_switch_to_guest(), there is not much to do
> > - * as SEAMCALL/SEAMRET calls take care of most of save and restore.
> > - */
> > -void tdx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
> > -{
> > - struct vcpu_vt *vt = to_vt(vcpu);
> > -
> > - if (vt->guest_state_loaded)
> > - return;
> > -
> > - if (likely(is_64bit_mm(current->mm)))
> > - vt->msr_host_kernel_gs_base = current->thread.gsbase;
> > - else
> > - vt->msr_host_kernel_gs_base = read_msr(MSR_KERNEL_GS_BASE);
> > -
> > - vt->guest_state_loaded = true;
> > -}
> > -
> > struct tdx_uret_msr {
> > u32 msr;
> > unsigned int slot;
> > @@ -795,19 +776,38 @@ static struct tdx_uret_msr tdx_uret_msrs[] = {
> > {.msr = MSR_TSC_AUX,},
> > };
> >
> > -static void tdx_user_return_msr_update_cache(void)
> > +void tdx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
> > {
> > + struct vcpu_vt *vt = to_vt(vcpu);
> > int i;
> >
> > + if (vt->guest_state_loaded)
> > + return;
> > +
> > + if (likely(is_64bit_mm(current->mm)))
> > + vt->msr_host_kernel_gs_base = current->thread.gsbase;
> > + else
> > + vt->msr_host_kernel_gs_base = read_msr(MSR_KERNEL_GS_BASE);
> > +
> > + vt->guest_state_loaded = true;
> > +
> > + /*
> > + * Explicitly set user-return MSRs that are clobbered by the TDX-Module
> > + * if VP.ENTER succeeds, i.e. on TD-Exit, with the values that would be
> > + * written by the TDX-Module. Don't rely on the TDX-Module to actually
> > + * clobber the MSRs, as the contract is poorly defined and not upheld.
> > + * E.g. the TDX-Module will synthesize an EPT Violation without doing
> > + * VM-Enter if it suspects a zero-step attack, and never "restore" VMM
> > + * state.
> > + */
> > for (i = 0; i < ARRAY_SIZE(tdx_uret_msrs); i++)
> > - kvm_user_return_msr_update_cache(tdx_uret_msrs[i].slot,
> > - tdx_uret_msrs[i].defval);
> > + kvm_set_user_return_msr(tdx_uret_msrs[i].slot,
> > + tdx_uret_msrs[i].defval, -1ull);
> > }
> >
> > static void tdx_prepare_switch_to_host(struct kvm_vcpu *vcpu)
> > {
> > struct vcpu_vt *vt = to_vt(vcpu);
> > - struct vcpu_tdx *tdx = to_tdx(vcpu);
> >
> > if (!vt->guest_state_loaded)
> > return;
> > @@ -815,11 +815,6 @@ static void tdx_prepare_switch_to_host(struct kvm_vcpu *vcpu)
> > ++vcpu->stat.host_state_reload;
> > wrmsrl(MSR_KERNEL_GS_BASE, vt->msr_host_kernel_gs_base);
> >
> > - if (tdx->guest_entered) {
> > - tdx_user_return_msr_update_cache();
> > - tdx->guest_entered = false;
> > - }
> > -
> > vt->guest_state_loaded = false;
> > }
> >
> > @@ -1059,7 +1054,6 @@ fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> > update_debugctlmsr(vcpu->arch.host_debugctl);
> >
> > tdx_load_host_xsave_state(vcpu);
> > - tdx->guest_entered = true;
> >
> > vcpu->arch.regs_avail &= TDX_REGS_AVAIL_SET;
> >
> > diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> > index ca39a9391db1..7f258870dc41 100644
> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -67,7 +67,6 @@ struct vcpu_tdx {
> > u64 vp_enter_ret;
> >
> > enum vcpu_tdx_state state;
> > - bool guest_entered;
> >
> > u64 map_gpa_next;
> > u64 map_gpa_end;
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index b4b5d2d09634..639589af7cbe 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -681,15 +681,6 @@ int kvm_set_user_return_msr(unsigned slot, u64 value, u64 mask)
> > }
> > EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_set_user_return_msr);
> >
> > -void kvm_user_return_msr_update_cache(unsigned int slot, u64 value)
> > -{
> > - struct kvm_user_return_msrs *msrs = this_cpu_ptr(user_return_msrs);
> > -
> > - msrs->values[slot].curr = value;
> > - kvm_user_return_register_notifier(msrs);
> > -}
> > -EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_user_return_msr_update_cache);
> > -
> > u64 kvm_get_user_return_msr(unsigned int slot)
> > {
> > return this_cpu_ptr(user_return_msrs)->values[slot].curr;
> > --
> > 2.51.1.930.gacf6e81ea2-goog
> >
Powered by blists - more mailing lists