linux-kernel - Re: [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Oct 2022 23:18:38 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check

On Tue, Oct 18, 2022, Vitaly Kuznetsov wrote:
> When a new feature gets implemented in KVM, EVMCS1_UNSUPPORTED_* defines
> need to be adjusted to avoid the situation when the feature is exposed
> to the guest but there's no corresponding eVMCS field[s] for it. This
> is not obvious and fragile.

Eh, either way is fragile, the only difference is what goes wrong when it breaks.

At the risk of making this overly verbose, what about requiring developers to
explicitly define whether or not a new control is support?  E.g. keep the
EVMCS1_UNSUPPORTED_* and then add compile-time assertions to verify that every
feature that is REQUIRED | OPTIONAL is SUPPORTED | UNSUPPORTED.

That way the eVMCS "supported" controls don't need to include the ALWAYSON
controls, and anytime someone adds a new control, they'll have to stop and think
about eVMCS.

I think we'll still want (need?) the runtime sanitization, but this might allow
catching at least some cases without needing to wait until a control actually gets
exposed.

E.g. possibly with more macro magic to reduce the boilerplate

diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index d8b23c96d627..190932edcc02 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -422,6 +422,10 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *
        u32 ctl_high = (u32)(*pdata >> 32);
        u32 unsupported_ctrls;
 
+       BUILD_BUG_ON((EVMCS1_SUPPORTED_PINCTRL | EVMCS1_UNSUPPORTED_PINCTRL) !=
+                    (KVM_REQUIRED_VMX_PIN_BASED_VM_EXEC_CONTROL |
+                     KVM_OPTIONAL_VMX_PIN_BASED_VM_EXEC_CONTROL));
+
        /*
         * Hyper-V 2016 and 2019 try using these features even when eVMCS
         * is enabled but there are no corresponding fields.
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index 6f746ef3c038..58d77afe9d57 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h
@@ -48,6 +48,11 @@ DECLARE_STATIC_KEY_FALSE(enable_evmcs);
  */
 #define EVMCS1_UNSUPPORTED_PINCTRL (PIN_BASED_POSTED_INTR | \
                                    PIN_BASED_VMX_PREEMPTION_TIMER)
+#define EVMCS1_SUPPORTED_PINCTRL                                       \
+       (PIN_BASED_EXT_INTR_MASK |                                      \
+        PIN_BASED_NMI_EXITING |                                        \
+        PIN_BASED_VIRTUAL_NMIS)
+
 #define EVMCS1_UNSUPPORTED_EXEC_CTRL (CPU_BASED_ACTIVATE_TERTIARY_CONTROLS)
 #define EVMCS1_UNSUPPORTED_2NDEXEC                                     \
        (SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY |                         \