Sean Christopherson <seanjc@xxxxxxxxxx> writes: > On Wed, Jan 18, 2023, Alexandru Matei wrote: >> KVM enables 'Enlightened VMCS' and 'Enlightened MSR Bitmap' when running as >> a nested hypervisor on top of Hyper-V. When MSR bitmap is updated, >> evmcs_touch_msr_bitmap function uses current_vmcs per-cpu variable to mark >> that the msr bitmap was changed. >> >> vmx_vcpu_create() modifies the msr bitmap via vmx_disable_intercept_for_msr >> -> vmx_msr_bitmap_l01_changed which in the end calls this function. The >> function checks for current_vmcs if it is null but the check is >> insufficient because current_vmcs is not initialized. Because of this, the >> code might incorrectly write to the structure pointed by current_vmcs value >> left by another task. Preemption is not disabled so the current task can >> also be preempted and moved to another CPU while current_vmcs is accessed >> multiple times from evmcs_touch_msr_bitmap() which leads to crash. >> >> To fix this problem, this patch moves vmx_disable_intercept_for_msr calls >> before init_vmcs call in __vmx_vcpu_reset(), as ->vcpu_reset() is invoked >> after the vCPU is properly loaded via ->vcpu_load() and current_vmcs is >> initialized. > > IMO, moving the calls is a band-aid and doesn't address the underlying bug. I > don't see any reason why the Hyper-V code should use a per-cpu pointer in this > case. It makes sense when replacing VMX sequences that operate on the VMCS, e.g. > VMREAD, VMWRITE, etc., but for operations that aren't direct replacements for VMX > instructions I think we should have a rule that Hyper-V isn't allowed to touch the > per-cpu pointer. > > E.g. in this case it's trivial to pass down the target (completely untested). > > Vitaly? Mid-air collision detected) I've just suggested a very similar approach but instead of 'vmx->vmcs01.vmcs' I've suggested using 'vmx->loaded_vmcs->vmcs': in case we're running L2 and loaded VMCS is 'vmcs02', I think we still need to touch the clean field indicating that MSR-Bitmap has changed. Equally untested :-) > > > --- > arch/x86/kvm/vmx/hyperv.h | 12 +++++++----- > arch/x86/kvm/vmx/vmx.c | 2 +- > 2 files changed, 8 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/vmx/hyperv.h b/arch/x86/kvm/vmx/hyperv.h > index ab08a9b9ab7d..ad16b52766bb 100644 > --- a/arch/x86/kvm/vmx/hyperv.h > +++ b/arch/x86/kvm/vmx/hyperv.h > @@ -250,13 +250,15 @@ static inline u16 evmcs_read16(unsigned long field) > return *(u16 *)((char *)current_evmcs + offset); > } > > -static inline void evmcs_touch_msr_bitmap(void) > +static inline void evmcs_touch_msr_bitmap(struct vcpu_vmx *vmx) > { > - if (unlikely(!current_evmcs)) > + struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs; > + > + if (WARN_ON_ONCE(!evmcs)) > return; > > - if (current_evmcs->hv_enlightenments_control.msr_bitmap) > - current_evmcs->hv_clean_fields &= > + if (evmcs->hv_enlightenments_control.msr_bitmap) > + evmcs->hv_clean_fields &= > ~HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP; > } > > @@ -280,7 +282,7 @@ static inline u64 evmcs_read64(unsigned long field) { return 0; } > static inline u32 evmcs_read32(unsigned long field) { return 0; } > static inline u16 evmcs_read16(unsigned long field) { return 0; } > static inline void evmcs_load(u64 phys_addr) {} > -static inline void evmcs_touch_msr_bitmap(void) {} > +static inline void evmcs_touch_msr_bitmap(struct vcpu_vmx *vmx) {} > #endif /* IS_ENABLED(CONFIG_HYPERV) */ > > #define EVMPTR_INVALID (-1ULL) > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index c788aa382611..6ed6f52aad0c 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -3937,7 +3937,7 @@ static void vmx_msr_bitmap_l01_changed(struct vcpu_vmx *vmx) > * bitmap has changed. > */ > if (static_branch_unlikely(&enable_evmcs)) > - evmcs_touch_msr_bitmap(); > + evmcs_touch_msr_bitmap(vmx); > > vmx->nested.force_msr_bitmap_recalc = true; > } > > base-commit: 6e9a476ea49d43a27b42004cfd7283f128494d1d -- Vitaly