| Message ID | 20221130230934.1014142-11-seanjc@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | KVM: Rework kvm_init() and hardware enabling | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Guessing tree name failed |
Sean Christopherson <seanjc@google.com> writes: > Reset the eVMCS controls in the per-CPU VP assist page during hardware > disabling instead of waiting until kvm-intel's module exit. The controls > are activated if and only if KVM creates a VM, i.e. don't need to be > reset if hardware is never enabled. > > Doing the reset during hardware disabling will naturally fix a potential > NULL pointer deref bug once KVM disables CPU hotplug while enabling and > disabling hardware (which is necessary to fix a variety of bugs). If the > kernel is running as the root partition, the VP assist page is unmapped > during CPU hot unplug, and so KVM's clearing of the eVMCS controls needs > to occur with CPU hot(un)plug disabled, otherwise KVM could attempt to > write to a CPU's VP assist page after it's unmapped. > > Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/vmx/vmx.c | 50 +++++++++++++++++++++++++----------------- > 1 file changed, 30 insertions(+), 20 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index cea8c07f5229..d85d175dca70 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -551,6 +551,33 @@ static int hv_enable_l2_tlb_flush(struct kvm_vcpu *vcpu) > return 0; > } > > +static void hv_reset_evmcs(void) > +{ > + struct hv_vp_assist_page *vp_ap; > + > + if (!static_branch_unlikely(&enable_evmcs)) > + return; > + > + /* > + * KVM should enable eVMCS if and only if all CPUs have a VP assist > + * page, and should reject CPU onlining if eVMCS is enabled the CPU > + * doesn't have a VP assist page allocated. > + */ > + vp_ap = hv_get_vp_assist_page(smp_processor_id()); > + if (WARN_ON_ONCE(!vp_ap)) > + return; > + In case my understanding is correct, this may actually get triggered for Hyper-V root partition: vmx_hardware_disable() gets called from kvm_dying_cpu() which has its own CPUHP_AP_KVM_STARTING stage. VP page unmapping happens in hv_cpu_die() which uses generic CPUHP_AP_ONLINE_DYN (happens first on CPU oflining AFAIR). I believe we need to introduce a new CPUHP_AP_HYPERV_STARTING stage and put it before CPUHP_AP_KVM_STARTING so it happens after it upon offlining. The issue is likely theoretical as Hyper-V root partition is a very special case, I'm not sure whether KVM is used there and whether CPU offlining is possible. In any case, WARN_ON_ONCE() is much better than NULL pointer dereference we have now :-) > + /* > + * Reset everything to support using non-enlightened VMCS access later > + * (e.g. when we reload the module with enlightened_vmcs=0) > + */ > + vp_ap->nested_control.features.directhypercall = 0; > + vp_ap->current_nested_vmcs = 0; > + vp_ap->enlighten_vmentry = 0; > +} > + > +#else /* IS_ENABLED(CONFIG_HYPERV) */ > +static void hv_reset_evmcs(void) {} > #endif /* IS_ENABLED(CONFIG_HYPERV) */ > > /* > @@ -2496,6 +2523,8 @@ static void vmx_hardware_disable(void) > if (cpu_vmxoff()) > kvm_spurious_fault(); > > + hv_reset_evmcs(); > + > intel_pt_handle_vmx(0); > } > > @@ -8462,27 +8491,8 @@ static void vmx_exit(void) > kvm_exit(); > > #if IS_ENABLED(CONFIG_HYPERV) > - if (static_branch_unlikely(&enable_evmcs)) { > - int cpu; > - struct hv_vp_assist_page *vp_ap; > - /* > - * Reset everything to support using non-enlightened VMCS > - * access later (e.g. when we reload the module with > - * enlightened_vmcs=0) > - */ > - for_each_online_cpu(cpu) { > - vp_ap = hv_get_vp_assist_page(cpu); > - > - if (!vp_ap) > - continue; > - > - vp_ap->nested_control.features.directhypercall = 0; > - vp_ap->current_nested_vmcs = 0; > - vp_ap->enlighten_vmentry = 0; > - } > - > + if (static_branch_unlikely(&enable_evmcs)) > static_branch_disable(&enable_evmcs); > - } > #endif > vmx_cleanup_l1d_flush(); Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index cea8c07f5229..d85d175dca70 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -551,6 +551,33 @@ static int hv_enable_l2_tlb_flush(struct kvm_vcpu *vcpu) return 0; } +static void hv_reset_evmcs(void) +{ + struct hv_vp_assist_page *vp_ap; + + if (!static_branch_unlikely(&enable_evmcs)) + return; + + /* + * KVM should enable eVMCS if and only if all CPUs have a VP assist + * page, and should reject CPU onlining if eVMCS is enabled the CPU + * doesn't have a VP assist page allocated. + */ + vp_ap = hv_get_vp_assist_page(smp_processor_id()); + if (WARN_ON_ONCE(!vp_ap)) + return; + + /* + * Reset everything to support using non-enlightened VMCS access later + * (e.g. when we reload the module with enlightened_vmcs=0) + */ + vp_ap->nested_control.features.directhypercall = 0; + vp_ap->current_nested_vmcs = 0; + vp_ap->enlighten_vmentry = 0; +} + +#else /* IS_ENABLED(CONFIG_HYPERV) */ +static void hv_reset_evmcs(void) {} #endif /* IS_ENABLED(CONFIG_HYPERV) */ /* @@ -2496,6 +2523,8 @@ static void vmx_hardware_disable(void) if (cpu_vmxoff()) kvm_spurious_fault(); + hv_reset_evmcs(); + intel_pt_handle_vmx(0); } @@ -8462,27 +8491,8 @@ static void vmx_exit(void) kvm_exit(); #if IS_ENABLED(CONFIG_HYPERV) - if (static_branch_unlikely(&enable_evmcs)) { - int cpu; - struct hv_vp_assist_page *vp_ap; - /* - * Reset everything to support using non-enlightened VMCS - * access later (e.g. when we reload the module with - * enlightened_vmcs=0) - */ - for_each_online_cpu(cpu) { - vp_ap = hv_get_vp_assist_page(cpu); - - if (!vp_ap) - continue; - - vp_ap->nested_control.features.directhypercall = 0; - vp_ap->current_nested_vmcs = 0; - vp_ap->enlighten_vmentry = 0; - } - + if (static_branch_unlikely(&enable_evmcs)) static_branch_disable(&enable_evmcs); - } #endif vmx_cleanup_l1d_flush();
Reset the eVMCS controls in the per-CPU VP assist page during hardware disabling instead of waiting until kvm-intel's module exit. The controls are activated if and only if KVM creates a VM, i.e. don't need to be reset if hardware is never enabled. Doing the reset during hardware disabling will naturally fix a potential NULL pointer deref bug once KVM disables CPU hotplug while enabling and disabling hardware (which is necessary to fix a variety of bugs). If the kernel is running as the root partition, the VP assist page is unmapped during CPU hot unplug, and so KVM's clearing of the eVMCS controls needs to occur with CPU hot(un)plug disabled, otherwise KVM could attempt to write to a CPU's VP assist page after it's unmapped. Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/vmx/vmx.c | 50 +++++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 20 deletions(-)