Re: [PATCH] KVM: VMX: Fix crash due to uninitialized current_vmcs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux