| 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: <24c39bb2f1e97ef2e6207894cb62e456bededce0.camel@redhat.com>
Date: Wed, 21 May 2025 16:43:26 -0400
From: mlevitsk@...hat.com
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>, Borislav
Petkov <bp@...en8.de>, x86@...nel.org, Ingo Molnar <mingo@...hat.com>,
linux-kernel@...r.kernel.org, "H. Peter Anvin" <hpa@...or.com>, Paolo
Bonzini <pbonzini@...hat.com>, Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH v4 4/4] x86: KVM: VMX: preserve DEBUGCTLMSR_FREEZE_IN_SMM
On Tue, 2025-05-20 at 15:57 -0700, Sean Christopherson wrote:
> KVM: VMX:
>
> On Wed, May 14, 2025, Maxim Levitsky wrote:
> > Pass through the host's DEBUGCTL.DEBUGCTLMSR_FREEZE_IN_SMM to the guest
> > GUEST_IA32_DEBUGCTL without the guest seeing this value.
> >
> > Since the value of the host DEBUGCTL can in theory change between VM runs,
> > check if has changed, and if yes, then reload the GUEST_IA32_DEBUGCTL with
> > the new value.
>
> Please split this into two patches. Add vmx_guest_debugctl_{read,write}(), then
> land the FREEZE_IN_SMM change on top. Adding the helpers should be a nop and
> thus trivial to review, and similarly the DEBUGCTLMSR_FREEZE_IN_SMM change is
> actually pretty small. But combined, this patch is annoying to review because
> there's a lot of uninteresting diff to wade through to get at the FREEZE_IN_SMM
> logic.
No problem.
>
> > Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> > ---
> > arch/x86/include/asm/kvm_host.h | 1 +
> > arch/x86/kvm/vmx/nested.c | 4 ++--
> > arch/x86/kvm/vmx/vmx.c | 22 +++++++++++++++++++---
> > arch/x86/kvm/vmx/vmx.h | 2 ++
> > arch/x86/kvm/x86.c | 7 +++++--
> > 5 files changed, 29 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index d2ad31a1628e..2e7e4a8b392e 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -1673,6 +1673,7 @@ static inline u16 kvm_lapic_irq_dest_mode(bool dest_mode_logical)
> > enum kvm_x86_run_flags {
> > KVM_RUN_FORCE_IMMEDIATE_EXIT = BIT(0),
> > KVM_RUN_LOAD_GUEST_DR6 = BIT(1),
> > + KVM_RUN_LOAD_DEBUGCTL = BIT(2),
> > };
> >
> > struct kvm_x86_ops {
>
> ...
>
> > @@ -7368,6 +7381,9 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> > if (run_flags & KVM_RUN_LOAD_GUEST_DR6)
> > set_debugreg(vcpu->arch.dr6, 6);
> >
> > + if (run_flags & KVM_RUN_LOAD_DEBUGCTL)
> > + vmx_guest_debugctl_write(vcpu, vmx_guest_debugctl_read());
> > +
> > /*
> > * Refresh vmcs.HOST_CR3 if necessary. This must be done immediately
> > * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
> > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > index 1b80479505d3..5ddedf73392b 100644
> > --- a/arch/x86/kvm/vmx/vmx.h
> > +++ b/arch/x86/kvm/vmx/vmx.h
> > @@ -416,6 +416,8 @@ static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
> >
> > void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
> > u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
> > +void vmx_guest_debugctl_write(struct kvm_vcpu *vcpu, u64 val);
> > +u64 vmx_guest_debugctl_read(void);
>
> I vote to make these static inlines, I don't see any reason to bury them in vmx.c
No problem as well.
>
> > /*
> > * Note, early Intel manuals have the write-low and read-high bitmap offsets
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 684b8047e0f2..a85078dfa36d 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -10752,7 +10752,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > dm_request_for_irq_injection(vcpu) &&
> > kvm_cpu_accept_dm_intr(vcpu);
> > fastpath_t exit_fastpath;
> > - u64 run_flags;
> > + u64 run_flags, host_debug_ctl;
> >
> > bool req_immediate_exit = false;
> >
> > @@ -11024,7 +11024,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > set_debugreg(0, 7);
> > }
> >
> > - vcpu->arch.host_debugctl = get_debugctlmsr();
> > + host_debug_ctl = get_debugctlmsr();
>
> This can probably just be debug_ctl to shorten the lines, I don't see a strong
> need to clarify it's the host's value since all accesses are clustered together.
>
> > + if (host_debug_ctl != vcpu->arch.host_debugctl)
> > + run_flags |= KVM_RUN_LOAD_DEBUGCTL;
> > + vcpu->arch.host_debugctl = host_debug_ctl;
>
> Argh, the TDX series didn't get refreshed (or maybe it got poorly rebased), and
> now there's a redundant and confusing "host_debugctlmsr" field in vcpu_vt. Can
> you slot in the below? It's not urgent enough to warrant posting separately,
> and handling TDX in this series would get a bit wonky if TDX uses a different
> snapshot.
>
> The reason I say that TDX will get wonky is also why I think the "are bits
> changing?" check in x86.c needs to be precise. KVM_RUN_LOAD_DEBUGCTL should
> *never* be set for TDX and SVM, and so they should WARN instead of silently
> doing nothing. But to do that without generating false positives, the common
> check needs to be precise.
>
> I was going to say we could throw a mask in kvm_x86_ops, but TDX throws a wrench
> in that idea. Aha! Actually, we can still use kvm_x86_ops. TDX can be exempted
> via guest_state_protected. E.g. in common x86:
>
> debug_ctl = get_debugctlmsr();
> if (((debug_ctl ^ vcpu->arch.host_debugctl) & kvm_x86_ops.HOST_DEBUGCTL_MASK) &&
> !vcpu->arch.guest_state_protected)
> run_flags |= KVM_RUN_LOAD_DEBUGCTL;
> vcpu->arch.host_debugctl = debug_ctl;
Makes sense. I need to remember that TDX has arrived so I have to take it
in the account from now on.
All right, thanks for the review, I will post a new patch series soon.
Best regards,
Maxim Levitsky
>
> --
> From: Sean Christopherson <seanjc@...gle.com>
> Date: Tue, 20 May 2025 15:37:41 -0700
> Subject: [PATCH] KVM: TDX: Use kvm_arch_vcpu.host_debugctl to restore the
> host's DEBUGCTL
>
> Use the kvm_arch_vcpu.host_debugctl snapshot to restore DEBUGCTL after
> running a TD vCPU. The final TDX series rebase was mishandled, likely due
> to commit fb71c7959356 ("KVM: x86: Snapshot the host's DEBUGCTL in common
> x86") deleting the same line of code from vmx.h, i.e. creating a semantic
> conflict of sorts, but no syntactic conflict.
>
> Using the version in kvm_vcpu_arch picks up the ulong => u64 fix (which
> isn't relevant to TDX) as well as the IRQ fix from commit 189ecdb3e112
> ("KVM: x86: Snapshot the host's DEBUGCTL after disabling IRQs").
>
> Link: https://lore.kernel.org/all/20250307212053.2948340-10-pbonzini@redhat.com
> Cc: Adrian Hunter <adrian.hunter@...el.com>
> Fixes: 8af099037527 ("KVM: TDX: Save and restore IA32_DEBUGCTL")
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
> arch/x86/kvm/vmx/common.h | 2 --
> arch/x86/kvm/vmx/tdx.c | 6 ++----
> 2 files changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/common.h b/arch/x86/kvm/vmx/common.h
> index 8f46a06e2c44..66454bead202 100644
> --- a/arch/x86/kvm/vmx/common.h
> +++ b/arch/x86/kvm/vmx/common.h
> @@ -53,8 +53,6 @@ struct vcpu_vt {
> #ifdef CONFIG_X86_64
> u64 msr_host_kernel_gs_base;
> #endif
> -
> - unsigned long host_debugctlmsr;
> };
>
> #ifdef CONFIG_KVM_INTEL_TDX
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 7dbfad28debc..84b2922b8119 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -778,8 +778,6 @@ void tdx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
> else
> vt->msr_host_kernel_gs_base = read_msr(MSR_KERNEL_GS_BASE);
>
> - vt->host_debugctlmsr = get_debugctlmsr();
> -
> vt->guest_state_loaded = true;
> }
>
> @@ -1056,8 +1054,8 @@ fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
>
> tdx_vcpu_enter_exit(vcpu);
>
> - if (vt->host_debugctlmsr & ~TDX_DEBUGCTL_PRESERVED)
> - update_debugctlmsr(vt->host_debugctlmsr);
> + if (vcpu->arch.host_debugctl & ~TDX_DEBUGCTL_PRESERVED)
> + update_debugctlmsr(vcpu->arch.host_debugctl);
>
> tdx_load_host_xsave_state(vcpu);
> tdx->guest_entered = true;
>
> base-commit: 475a02020ac2de6b10e85de75e79833139b556e0
> --
>
Powered by blists - more mailing lists