| 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: <aC0IwYfNvuo_vUDU@google.com>
Date: Tue, 20 May 2025 15:57:05 -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>,
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
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.
> 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
> /*
> * 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;
--
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