| 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: <aAgpD_5BI6ZcCN29@google.com>
Date: Tue, 22 Apr 2025 16:41:03 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Maxim Levitsky <mlevitsk@...hat.com>
Cc: kvm@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
Paolo Bonzini <pbonzini@...hat.com>, x86@...nel.org,
Dave Hansen <dave.hansen@...ux.intel.com>, Ingo Molnar <mingo@...hat.com>,
linux-kernel@...r.kernel.org, "H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH 3/3] x86: KVM: VMX: preserve host's DEBUGCTLMSR_FREEZE_IN_SMM
while in the guest mode
On Tue, Apr 15, 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.
>
> Note that in the future we might allow the guest to set this bit as well,
> when we implement PMU freezing on VM own, virtual SMM entry.
>
> 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 of the host portion of it (currently only the
> DEBUGCTLMSR_FREEZE_IN_SMM bit)
No, it can't. DEBUGCTLMSR_FREEZE_IN_SMM can be toggled via IPI callback, but
IRQs are disabled for the entirety of the inner run loop. And if I'm somehow
wrong, this change movement absolutely belongs in a separate patch.
> Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> ---
> arch/x86/kvm/svm/svm.c | 2 ++
> arch/x86/kvm/vmx/vmx.c | 28 +++++++++++++++++++++++++++-
> arch/x86/kvm/x86.c | 2 --
> 3 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index cc1c721ba067..fda0660236d8 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4271,6 +4271,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
> svm->vmcb->save.rsp = vcpu->arch.regs[VCPU_REGS_RSP];
> svm->vmcb->save.rip = vcpu->arch.regs[VCPU_REGS_RIP];
>
> + vcpu->arch.host_debugctl = get_debugctlmsr();
> +
> /*
> * Disable singlestep if we're injecting an interrupt/exception.
> * We don't want our modified rflags to be pushed on the stack where
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index c9208a4acda4..e0bc31598d60 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2194,6 +2194,17 @@ static u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated
> return debugctl;
> }
>
> +static u64 vmx_get_host_preserved_debugctl(struct kvm_vcpu *vcpu)
No, just open code handling DEBUGCTLMSR_FREEZE_IN_SMM, or make it a #define.
I'm not remotely convinced that we'll ever want to emulate DEBUGCTLMSR_FREEZE_IN_SMM,
and trying to plan for that possibility and adds complexity for no immediate value.
> +{
> + /*
> + * Bits of host's DEBUGCTL that we should preserve while the guest is
> + * running.
> + *
> + * Some of those bits might still be emulated for the guest own use.
> + */
> + return DEBUGCTLMSR_FREEZE_IN_SMM;
>
> u64 vmx_get_guest_debugctl(struct kvm_vcpu *vcpu)
> {
> return to_vmx(vcpu)->msr_ia32_debugctl;
> @@ -2202,9 +2213,11 @@ u64 vmx_get_guest_debugctl(struct kvm_vcpu *vcpu)
> static void __vmx_set_guest_debugctl(struct kvm_vcpu *vcpu, u64 data)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> + u64 host_mask = vmx_get_host_preserved_debugctl(vcpu);
>
> vmx->msr_ia32_debugctl = data;
> - vmcs_write64(GUEST_IA32_DEBUGCTL, data);
> + vmcs_write64(GUEST_IA32_DEBUGCTL,
> + (vcpu->arch.host_debugctl & host_mask) | (data & ~host_mask));
> }
>
> bool vmx_set_guest_debugctl(struct kvm_vcpu *vcpu, u64 data, bool host_initiated)
> @@ -2232,6 +2245,7 @@ bool vmx_set_guest_debugctl(struct kvm_vcpu *vcpu, u64 data, bool host_initiated
> return true;
> }
>
> +
Spurious newline.
> /*
> * Writes msr value into the appropriate "register".
> * Returns 0 on success, non-0 otherwise.
> @@ -7349,6 +7363,7 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> unsigned long cr3, cr4;
> + u64 old_debugctl;
>
> /* Record the guest's net vcpu time for enforced NMI injections. */
> if (unlikely(!enable_vnmi &&
> @@ -7379,6 +7394,17 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit)
> vmcs_write32(PLE_WINDOW, vmx->ple_window);
> }
>
> + old_debugctl = vcpu->arch.host_debugctl;
> + vcpu->arch.host_debugctl = get_debugctlmsr();
> +
> + /*
> + * In case the host DEBUGCTL had changed since the last time we
> + * read it, update the guest's GUEST_IA32_DEBUGCTL with
> + * the host's bits.
> + */
> + if (old_debugctl != vcpu->arch.host_debugctl)
This can and should be optimized to only do an update if a host-preserved bit
is toggled.
> + __vmx_set_guest_debugctl(vcpu, vmx->msr_ia32_debugctl);
I would rather have a helper that explicitly writes the VMCS field, not one that
sets the guest value *and* writes the VMCS field.
The usage in init_vmcs() doesn't need to write vmx->msr_ia32_debugctl because the
vCPU is zero allocated, and this usage doesn't change vmx->msr_ia32_debugctl.
So the only path that actually needs to modify vmx->msr_ia32_debugctl is
vmx_set_guest_debugctl().
> +
> /*
> * We did this in prepare_switch_to_guest, because it needs to
> * be within srcu_read_lock.
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 844e81ee1d96..05e866ed345d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -11020,8 +11020,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> set_debugreg(0, 7);
> }
>
> - vcpu->arch.host_debugctl = get_debugctlmsr();
> -
> guest_timing_enter_irqoff();
>
> for (;;) {
> --
> 2.26.3
>
Powered by blists - more mailing lists