On Tue, Apr 15, 2025, Maxim Levitsky wrote:
> Instead of reading and writing GUEST_IA32_DEBUGCTL vmcs field directly,
> wrap the logic with get/set functions.
Why? I know why the "set" helper is being added, but it needs to called out.
Please omit the getter entirely, it does nothing more than obfuscate a very
simple line of code.
> Also move the checks that the guest's supplied value is valid to the new
> 'set' function.
Please do this in a separate patch. There's no need to mix refactoring and
functional changes.
> In particular, the above change fixes a minor security issue in which L1
Bug, yes. Not sure it constitutes a meaningful security issue though.
> hypervisor could set the GUEST_IA32_DEBUGCTL, and eventually the host's
> MSR_IA32_DEBUGCTL
No, the lack of a consistency check allows the guest to set the MSR in hardware,
but that is not the host's value.
> to any value by performing a VM entry to L2 with VM_ENTRY_LOAD_DEBUG_CONTROLS
> set.
Any *legal* value. Setting completely unsupported bits will result in VM-Enter
failing with a consistency check VM-Exit.
> Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> ---
> arch/x86/kvm/vmx/nested.c | 15 +++++++---
> arch/x86/kvm/vmx/pmu_intel.c | 9 +++---
> arch/x86/kvm/vmx/vmx.c | 58 +++++++++++++++++++++++-------------
> arch/x86/kvm/vmx/vmx.h | 3 ++
> 4 files changed, 57 insertions(+), 28 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index e073e3008b16..b7686569ee09 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2641,6 +2641,7 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> struct hv_enlightened_vmcs *evmcs = nested_vmx_evmcs(vmx);
> bool load_guest_pdptrs_vmcs12 = false;
> + u64 new_debugctl;
>
> if (vmx->nested.dirty_vmcs12 || nested_vmx_is_evmptr12_valid(vmx)) {
> prepare_vmcs02_rare(vmx, vmcs12);
> @@ -2653,11 +2654,17 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
> if (vmx->nested.nested_run_pending &&
> (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS)) {
> kvm_set_dr(vcpu, 7, vmcs12->guest_dr7);
> - vmcs_write64(GUEST_IA32_DEBUGCTL, vmcs12->guest_ia32_debugctl);
> + new_debugctl = vmcs12->guest_ia32_debugctl;
> } else {
> kvm_set_dr(vcpu, 7, vcpu->arch.dr7);
> - vmcs_write64(GUEST_IA32_DEBUGCTL, vmx->nested.pre_vmenter_debugctl);
> + new_debugctl = vmx->nested.pre_vmenter_debugctl;
> }
> +
> + if (CC(!vmx_set_guest_debugctl(vcpu, new_debugctl, false))) {
The consistency check belongs in nested_vmx_check_guest_state(), only needs to
check the VM_ENTRY_LOAD_DEBUG_CONTROLS case, and should be posted as a separate
patch.
> + *entry_failure_code = ENTRY_FAIL_DEFAULT;
> + return -EINVAL;
> + }
> +
> +static void __vmx_set_guest_debugctl(struct kvm_vcpu *vcpu, u64 data)
> +{
> + vmcs_write64(GUEST_IA32_DEBUGCTL, data);
> +}
> +
> +bool vmx_set_guest_debugctl(struct kvm_vcpu *vcpu, u64 data, bool host_initiated)
> +{
> + u64 invalid = data & ~vmx_get_supported_debugctl(vcpu, host_initiated);
> +
> + if (invalid & (DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR)) {
> + kvm_pr_unimpl_wrmsr(vcpu, MSR_IA32_DEBUGCTLMSR, data);
> + data &= ~(DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR);
> + invalid &= ~(DEBUGCTLMSR_BTF|DEBUGCTLMSR_LBR);
> + }
> +
> + if (invalid)
> + return false;
> +
> + if (is_guest_mode(vcpu) && (get_vmcs12(vcpu)->vm_exit_controls &
> + VM_EXIT_SAVE_DEBUG_CONTROLS))
> + get_vmcs12(vcpu)->guest_ia32_debugctl = data;
> +
> + if (intel_pmu_lbr_is_enabled(vcpu) && !to_vmx(vcpu)->lbr_desc.event &&
> + (data & DEBUGCTLMSR_LBR))
> + intel_pmu_create_guest_lbr_event(vcpu);
> +
> + __vmx_set_guest_debugctl(vcpu, data);
> + return true;
Return 0/-errno, not true/false.
