[PATCH v2] KVM: VMX: Fix crash due to uninitialized current

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v2] KVM: VMX: Fix crash due to uninitialized current_vmcs
@ 2023-01-23 12:46 Alexandru Matei
  2023-01-23 13:24 ` Vitaly Kuznetsov
  0 siblings, 1 reply; 6+ messages in thread
From: Alexandru Matei @ 2023-01-23 12:46 UTC (permalink / raw)
  To: Paolo Bonzini, Vitaly Kuznetsov, Sean Christopherson
  Cc: kvm, Alexandru Matei, Mihai Petrisor, Viorel Canja

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, the current task can be
preempted and moved to another CPU while current_vmcs is accessed multiple
times from evmcs_touch_msr_bitmap() which leads to crash.

The manipulation of MSR bitmaps by callers happens only for vmcs01 so the
solution is to use vmx->vmcs01.vmcs instead of current_vmcs.

BUG: kernel NULL pointer dereference, address: 0000000000000338
PGD 4e1775067 P4D 0
Oops: 0002 [#1] PREEMPT SMP NOPTI
...
RIP: 0010:vmx_msr_bitmap_l01_changed+0x39/0x50 [kvm_intel]
...
Call Trace:
 vmx_disable_intercept_for_msr+0x36/0x260 [kvm_intel]
 vmx_vcpu_create+0xe6/0x540 [kvm_intel]
 ? __vmalloc_node+0x4a/0x70
 kvm_arch_vcpu_create+0x1d1/0x2e0 [kvm]
 kvm_vm_ioctl_create_vcpu+0x178/0x430 [kvm]
 ? __handle_mm_fault+0x3cb/0x750
 kvm_vm_ioctl+0x53f/0x790 [kvm]
 ? syscall_exit_work+0x11a/0x150
 ? syscall_exit_to_user_mode+0x12/0x30
 ? do_syscall_64+0x69/0x90
 ? handle_mm_fault+0xc5/0x2a0
 __x64_sys_ioctl+0x8a/0xc0
 do_syscall_64+0x5c/0x90
 ? exc_page_fault+0x62/0x150
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
---
v2:
  - pass (e)vmcs01 to evmcs_touch_msr_bitmap
  - use loaded_vmcs * instead of vcpu_vmx * to avoid
    including vmx.h which generates circular dependency

v1: https://lore.kernel.org/kvm/20230118141348.828-1-alexandru.matei@uipath.com/

 arch/x86/kvm/vmx/hyperv.h | 16 +++++++++++-----
 arch/x86/kvm/vmx/vmx.c    |  2 +-
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx/hyperv.h b/arch/x86/kvm/vmx/hyperv.h
index 571e7929d14e..132e32e57d2d 100644
--- a/arch/x86/kvm/vmx/hyperv.h
+++ b/arch/x86/kvm/vmx/hyperv.h
@@ -190,13 +190,19 @@ 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 loaded_vmcs *vmcs01)
 {
-	if (unlikely(!current_evmcs))
+	/*
+	 * Enlightened MSR Bitmap feature is enabled only for L1, i.e.
+	 * always operates on (e)vmcs01
+	 */
+	struct hv_enlightened_vmcs* evmcs = (void*)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;
 }
 
@@ -219,7 +225,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 loaded_vmcs *vmcs01) {}
 #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 fe5615fd8295..2a3be8e8a1bf 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3869,7 +3869,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->vmcs01);
 
 	vmx->nested.force_msr_bitmap_recalc = true;
 }
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] KVM: VMX: Fix crash due to uninitialized current_vmcs
  2023-01-23 12:46 [PATCH v2] KVM: VMX: Fix crash due to uninitialized current_vmcs Alexandru Matei
@ 2023-01-23 13:24 ` Vitaly Kuznetsov
  2023-01-23 14:11   ` Alexandru Matei
  0 siblings, 1 reply; 6+ messages in thread
From: Vitaly Kuznetsov @ 2023-01-23 13:24 UTC (permalink / raw)
  To: Alexandru Matei, Paolo Bonzini, Sean Christopherson
  Cc: kvm, Alexandru Matei, Mihai Petrisor, Viorel Canja

Alexandru Matei <alexandru.matei@uipath.com> writes:

> 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, the current task can be
> preempted and moved to another CPU while current_vmcs is accessed multiple
> times from evmcs_touch_msr_bitmap() which leads to crash.
>
> The manipulation of MSR bitmaps by callers happens only for vmcs01 so the
> solution is to use vmx->vmcs01.vmcs instead of current_vmcs.
>
> BUG: kernel NULL pointer dereference, address: 0000000000000338
> PGD 4e1775067 P4D 0
> Oops: 0002 [#1] PREEMPT SMP NOPTI
> ...
> RIP: 0010:vmx_msr_bitmap_l01_changed+0x39/0x50 [kvm_intel]
> ...
> Call Trace:
>  vmx_disable_intercept_for_msr+0x36/0x260 [kvm_intel]
>  vmx_vcpu_create+0xe6/0x540 [kvm_intel]
>  ? __vmalloc_node+0x4a/0x70
>  kvm_arch_vcpu_create+0x1d1/0x2e0 [kvm]
>  kvm_vm_ioctl_create_vcpu+0x178/0x430 [kvm]
>  ? __handle_mm_fault+0x3cb/0x750
>  kvm_vm_ioctl+0x53f/0x790 [kvm]
>  ? syscall_exit_work+0x11a/0x150
>  ? syscall_exit_to_user_mode+0x12/0x30
>  ? do_syscall_64+0x69/0x90
>  ? handle_mm_fault+0xc5/0x2a0
>  __x64_sys_ioctl+0x8a/0xc0
>  do_syscall_64+0x5c/0x90
>  ? exc_page_fault+0x62/0x150
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
> ---
> v2:
>   - pass (e)vmcs01 to evmcs_touch_msr_bitmap
>   - use loaded_vmcs * instead of vcpu_vmx * to avoid
>     including vmx.h which generates circular dependency
>
> v1: https://lore.kernel.org/kvm/20230118141348.828-1-alexandru.matei@uipath.com/
>
>  arch/x86/kvm/vmx/hyperv.h | 16 +++++++++++-----
>  arch/x86/kvm/vmx/vmx.c    |  2 +-
>  2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/hyperv.h b/arch/x86/kvm/vmx/hyperv.h
> index 571e7929d14e..132e32e57d2d 100644
> --- a/arch/x86/kvm/vmx/hyperv.h
> +++ b/arch/x86/kvm/vmx/hyperv.h
> @@ -190,13 +190,19 @@ 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 loaded_vmcs *vmcs01)

Personally, I would've followed Sean's suggestion and passed "struct
vcpu_vmx *vmx" as 'loaded_vmcs' here is a bit ambiguos....

>  {
> -	if (unlikely(!current_evmcs))
> +	/*
> +	 * Enlightened MSR Bitmap feature is enabled only for L1, i.e.
> +	 * always operates on (e)vmcs01
> +	 */
> +	struct hv_enlightened_vmcs* evmcs = (void*)vmcs01->vmcs;

(Nit: a space between "void" and "*")

.. or, alternatively, you can directly pass 
(struct hv_enlightened_vmcs *)vmx->vmcs01.vmcs as e.g. 'current_evmcs'
and avoid the conversion here.

> +
> +	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;
>  }
>  
> @@ -219,7 +225,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 loaded_vmcs *vmcs01) {}
>  #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 fe5615fd8295..2a3be8e8a1bf 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -3869,7 +3869,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->vmcs01);
>  
>  	vmx->nested.force_msr_bitmap_recalc = true;
>  }

-- 
Vitaly


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] KVM: VMX: Fix crash due to uninitialized current_vmcs
  2023-01-23 13:24 ` Vitaly Kuznetsov
@ 2023-01-23 14:11   ` Alexandru Matei
  2023-01-23 15:57     ` Sean Christopherson
  0 siblings, 1 reply; 6+ messages in thread
From: Alexandru Matei @ 2023-01-23 14:11 UTC (permalink / raw)
  To: Vitaly Kuznetsov, Paolo Bonzini, Sean Christopherson
  Cc: kvm, Mihai Petrisor, Viorel Canja, Alexandru Matei

On 1/23/2023 3:24 PM, Vitaly Kuznetsov wrote:
> Alexandru Matei <alexandru.matei@uipath.com> writes:
> 
>> 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, the current task can be
>> preempted and moved to another CPU while current_vmcs is accessed multiple
>> times from evmcs_touch_msr_bitmap() which leads to crash.
>>
>> The manipulation of MSR bitmaps by callers happens only for vmcs01 so the
>> solution is to use vmx->vmcs01.vmcs instead of current_vmcs.
>>
>> BUG: kernel NULL pointer dereference, address: 0000000000000338
>> PGD 4e1775067 P4D 0
>> Oops: 0002 [#1] PREEMPT SMP NOPTI
>> ...
>> RIP: 0010:vmx_msr_bitmap_l01_changed+0x39/0x50 [kvm_intel]
>> ...
>> Call Trace:
>>  vmx_disable_intercept_for_msr+0x36/0x260 [kvm_intel]
>>  vmx_vcpu_create+0xe6/0x540 [kvm_intel]
>>  ? __vmalloc_node+0x4a/0x70
>>  kvm_arch_vcpu_create+0x1d1/0x2e0 [kvm]
>>  kvm_vm_ioctl_create_vcpu+0x178/0x430 [kvm]
>>  ? __handle_mm_fault+0x3cb/0x750
>>  kvm_vm_ioctl+0x53f/0x790 [kvm]
>>  ? syscall_exit_work+0x11a/0x150
>>  ? syscall_exit_to_user_mode+0x12/0x30
>>  ? do_syscall_64+0x69/0x90
>>  ? handle_mm_fault+0xc5/0x2a0
>>  __x64_sys_ioctl+0x8a/0xc0
>>  do_syscall_64+0x5c/0x90
>>  ? exc_page_fault+0x62/0x150
>>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
>> ---
>> v2:
>>   - pass (e)vmcs01 to evmcs_touch_msr_bitmap
>>   - use loaded_vmcs * instead of vcpu_vmx * to avoid
>>     including vmx.h which generates circular dependency
>>
>> v1: https://lore.kernel.org/kvm/20230118141348.828-1-alexandru.matei@uipath.com/
>>
>>  arch/x86/kvm/vmx/hyperv.h | 16 +++++++++++-----
>>  arch/x86/kvm/vmx/vmx.c    |  2 +-
>>  2 files changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/hyperv.h b/arch/x86/kvm/vmx/hyperv.h
>> index 571e7929d14e..132e32e57d2d 100644
>> --- a/arch/x86/kvm/vmx/hyperv.h
>> +++ b/arch/x86/kvm/vmx/hyperv.h
>> @@ -190,13 +190,19 @@ 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 loaded_vmcs *vmcs01)
> 
> Personally, I would've followed Sean's suggestion and passed "struct
> vcpu_vmx *vmx" as 'loaded_vmcs' here is a bit ambiguos....

I couldn't use vcpu_vmx *, it requires including vmx.h in hyperv.h 
and it generates a circular depedency which leads to compile errors.

> 
>>  {
>> -	if (unlikely(!current_evmcs))
>> +	/*
>> +	 * Enlightened MSR Bitmap feature is enabled only for L1, i.e.
>> +	 * always operates on (e)vmcs01
>> +	 */
>> +	struct hv_enlightened_vmcs* evmcs = (void*)vmcs01->vmcs;
> 
> (Nit: a space between "void" and "*")
> 
> .. or, alternatively, you can directly pass 
> (struct hv_enlightened_vmcs *)vmx->vmcs01.vmcs as e.g. 'current_evmcs'
> and avoid the conversion here.

OK, sounds good, I'll pass hv_enlightened_vmcs * directly.

> 
>> +
>> +	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;
>>  }
>>  
>> @@ -219,7 +225,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 loaded_vmcs *vmcs01) {}
>>  #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 fe5615fd8295..2a3be8e8a1bf 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -3869,7 +3869,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->vmcs01);
>>  
>>  	vmx->nested.force_msr_bitmap_recalc = true;
>>  }
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] KVM: VMX: Fix crash due to uninitialized current_vmcs
  2023-01-23 14:11   ` Alexandru Matei
@ 2023-01-23 15:57     ` Sean Christopherson
  2023-01-23 16:01       ` Sean Christopherson
  0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2023-01-23 15:57 UTC (permalink / raw)
  To: Alexandru Matei
  Cc: Vitaly Kuznetsov, Paolo Bonzini, kvm, Mihai Petrisor,
	Viorel Canja

On Mon, Jan 23, 2023, Alexandru Matei wrote:
> On 1/23/2023 3:24 PM, Vitaly Kuznetsov wrote:
> > Alexandru Matei <alexandru.matei@uipath.com> writes:
> > 
> >> 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, the current task can be
> >> preempted and moved to another CPU while current_vmcs is accessed multiple
> >> times from evmcs_touch_msr_bitmap() which leads to crash.
> >>
> >> The manipulation of MSR bitmaps by callers happens only for vmcs01 so the
> >> solution is to use vmx->vmcs01.vmcs instead of current_vmcs.
> >>
> >> BUG: kernel NULL pointer dereference, address: 0000000000000338
> >> PGD 4e1775067 P4D 0
> >> Oops: 0002 [#1] PREEMPT SMP NOPTI
> >> ...
> >> RIP: 0010:vmx_msr_bitmap_l01_changed+0x39/0x50 [kvm_intel]
> >> ...
> >> Call Trace:
> >>  vmx_disable_intercept_for_msr+0x36/0x260 [kvm_intel]
> >>  vmx_vcpu_create+0xe6/0x540 [kvm_intel]
> >>  ? __vmalloc_node+0x4a/0x70
> >>  kvm_arch_vcpu_create+0x1d1/0x2e0 [kvm]
> >>  kvm_vm_ioctl_create_vcpu+0x178/0x430 [kvm]
> >>  ? __handle_mm_fault+0x3cb/0x750
> >>  kvm_vm_ioctl+0x53f/0x790 [kvm]
> >>  ? syscall_exit_work+0x11a/0x150
> >>  ? syscall_exit_to_user_mode+0x12/0x30
> >>  ? do_syscall_64+0x69/0x90
> >>  ? handle_mm_fault+0xc5/0x2a0
> >>  __x64_sys_ioctl+0x8a/0xc0
> >>  do_syscall_64+0x5c/0x90
> >>  ? exc_page_fault+0x62/0x150
> >>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> >>
> >> Suggested-by: Sean Christopherson <seanjc@google.com>
> >> Signed-off-by: Alexandru Matei <alexandru.matei@uipath.com>
> >> ---
> >> v2:
> >>   - pass (e)vmcs01 to evmcs_touch_msr_bitmap
> >>   - use loaded_vmcs * instead of vcpu_vmx * to avoid
> >>     including vmx.h which generates circular dependency
> >>
> >> v1: https://lore.kernel.org/kvm/20230118141348.828-1-alexandru.matei@uipath.com/
> >>
> >>  arch/x86/kvm/vmx/hyperv.h | 16 +++++++++++-----
> >>  arch/x86/kvm/vmx/vmx.c    |  2 +-
> >>  2 files changed, 12 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/x86/kvm/vmx/hyperv.h b/arch/x86/kvm/vmx/hyperv.h
> >> index 571e7929d14e..132e32e57d2d 100644
> >> --- a/arch/x86/kvm/vmx/hyperv.h
> >> +++ b/arch/x86/kvm/vmx/hyperv.h
> >> @@ -190,13 +190,19 @@ 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 loaded_vmcs *vmcs01)
> > 
> > Personally, I would've followed Sean's suggestion and passed "struct
> > vcpu_vmx *vmx" as 'loaded_vmcs' here is a bit ambiguos....
> 
> I couldn't use vcpu_vmx *, it requires including vmx.h in hyperv.h 
> and it generates a circular depedency which leads to compile errors.
> 
> > 
> >>  {
> >> -	if (unlikely(!current_evmcs))
> >> +	/*
> >> +	 * Enlightened MSR Bitmap feature is enabled only for L1, i.e.
> >> +	 * always operates on (e)vmcs01
> >> +	 */
> >> +	struct hv_enlightened_vmcs* evmcs = (void*)vmcs01->vmcs;
> > 
> > (Nit: a space between "void" and "*")
> > 
> > .. or, alternatively, you can directly pass 
> > (struct hv_enlightened_vmcs *)vmx->vmcs01.vmcs as e.g. 'current_evmcs'
> > and avoid the conversion here.
> 
> OK, sounds good, I'll pass hv_enlightened_vmcs * directly.

Passing the eVMCS is silly, if we're going to bleed eVMCS details into vmx.c then
we should just commit and expose all details.  For this feature specifically, KVM
already handles the enabling in vmx.c / vmx_vcpu_create():

	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs) &&
	    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
		struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;

		evmcs->hv_enlightenments_control.msr_bitmap = 1;
	}

And if we handle this fully in vmx_msr_bitmap_l01_changed(), then there's no need
for a comment explaining that the feature is only enabled for vmcs01.

If we want to maintain better separate between VMX and Hyper-V code, then just make
the helper non-inline in hyperv.c, modifying MSR bitmaps will never be a hot path
for any sane guest.

I don't think I have a strong preference either way.  In a perfect world we'd keep
Hyper-V code separate, but practically speaking I think trying to move everything
into hyperv.c would result in far too many stubs and some weird function names.

Side topic, we should really have a wrapper for static_branch_unlikely(&enable_evmcs)
so that the static key can be defined iff CONFIG_HYPERV=y.  I'll send a patch.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] KVM: VMX: Fix crash due to uninitialized current_vmcs
  2023-01-23 15:57     ` Sean Christopherson
@ 2023-01-23 16:01       ` Sean Christopherson
  2023-01-23 16:57         ` Vitaly Kuznetsov
  0 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2023-01-23 16:01 UTC (permalink / raw)
  To: Alexandru Matei
  Cc: Vitaly Kuznetsov, Paolo Bonzini, kvm, Mihai Petrisor,
	Viorel Canja

On Mon, Jan 23, 2023, Sean Christopherson wrote:
> On Mon, Jan 23, 2023, Alexandru Matei wrote:
> > > .. or, alternatively, you can directly pass 
> > > (struct hv_enlightened_vmcs *)vmx->vmcs01.vmcs as e.g. 'current_evmcs'
> > > and avoid the conversion here.
> > 
> > OK, sounds good, I'll pass hv_enlightened_vmcs * directly.
> 
> Passing the eVMCS is silly, if we're going to bleed eVMCS details into vmx.c then
> we should just commit and expose all details.  For this feature specifically, KVM
> already handles the enabling in vmx.c / vmx_vcpu_create():
> 
> 	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs) &&
> 	    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
> 		struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;
> 
> 		evmcs->hv_enlightenments_control.msr_bitmap = 1;
> 	}
> 
> And if we handle this fully in vmx_msr_bitmap_l01_changed(), then there's no need
> for a comment explaining that the feature is only enabled for vmcs01.

Oh, and the sanity check on a null pointer also goes away.

> If we want to maintain better separate between VMX and Hyper-V code, then just make
> the helper non-inline in hyperv.c, modifying MSR bitmaps will never be a hot path
> for any sane guest.
> 
> I don't think I have a strong preference either way.  In a perfect world we'd keep
> Hyper-V code separate, but practically speaking I think trying to move everything
> into hyperv.c would result in far too many stubs and some weird function names.
> 
> Side topic, we should really have a wrapper for static_branch_unlikely(&enable_evmcs)
> so that the static key can be defined iff CONFIG_HYPERV=y.  I'll send a patch.

I.e.

---
 arch/x86/kvm/vmx/hyperv.h | 11 -----------
 arch/x86/kvm/vmx/vmx.c    |  9 +++++++--
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/vmx/hyperv.h b/arch/x86/kvm/vmx/hyperv.h
index ab08a9b9ab7d..bac614e40078 100644
--- a/arch/x86/kvm/vmx/hyperv.h
+++ b/arch/x86/kvm/vmx/hyperv.h
@@ -250,16 +250,6 @@ static inline u16 evmcs_read16(unsigned long field)
 	return *(u16 *)((char *)current_evmcs + offset);
 }
 
-static inline void evmcs_touch_msr_bitmap(void)
-{
-	if (unlikely(!current_evmcs))
-		return;
-
-	if (current_evmcs->hv_enlightenments_control.msr_bitmap)
-		current_evmcs->hv_clean_fields &=
-			~HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP;
-}
-
 static inline void evmcs_load(u64 phys_addr)
 {
 	struct hv_vp_assist_page *vp_ap =
@@ -280,7 +270,6 @@ 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) {}
 #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..ed4051b54412 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -3936,8 +3936,13 @@ static void vmx_msr_bitmap_l01_changed(struct vcpu_vmx *vmx)
 	 * 'Enlightened MSR Bitmap' feature L0 needs to know that MSR
 	 * bitmap has changed.
 	 */
-	if (static_branch_unlikely(&enable_evmcs))
-		evmcs_touch_msr_bitmap();
+	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs)) {
+		struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;
+
+		if (evmcs->hv_enlightenments_control.msr_bitmap)
+			evmcs->hv_clean_fields &=
+				~HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP;
+	}
 
 	vmx->nested.force_msr_bitmap_recalc = true;
 }

base-commit: 68bfbbf518a25856c2a3f07ea9d0c626f1b001fb
-- 


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] KVM: VMX: Fix crash due to uninitialized current_vmcs
  2023-01-23 16:01       ` Sean Christopherson
@ 2023-01-23 16:57         ` Vitaly Kuznetsov
  0 siblings, 0 replies; 6+ messages in thread
From: Vitaly Kuznetsov @ 2023-01-23 16:57 UTC (permalink / raw)
  To: Sean Christopherson, Alexandru Matei
  Cc: Paolo Bonzini, kvm, Mihai Petrisor, Viorel Canja

Sean Christopherson <seanjc@google.com> writes:

> On Mon, Jan 23, 2023, Sean Christopherson wrote:
>> On Mon, Jan 23, 2023, Alexandru Matei wrote:
>> > > .. or, alternatively, you can directly pass 
>> > > (struct hv_enlightened_vmcs *)vmx->vmcs01.vmcs as e.g. 'current_evmcs'
>> > > and avoid the conversion here.
>> > 
>> > OK, sounds good, I'll pass hv_enlightened_vmcs * directly.
>> 
>> Passing the eVMCS is silly, if we're going to bleed eVMCS details into vmx.c then
>> we should just commit and expose all details.  For this feature specifically, KVM
>> already handles the enabling in vmx.c / vmx_vcpu_create():
>> 
>> 	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs) &&
>> 	    (ms_hyperv.nested_features & HV_X64_NESTED_MSR_BITMAP)) {
>> 		struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;
>> 
>> 		evmcs->hv_enlightenments_control.msr_bitmap = 1;
>> 	}
>> 
>> And if we handle this fully in vmx_msr_bitmap_l01_changed(), then there's no need
>> for a comment explaining that the feature is only enabled for vmcs01.
>
> Oh, and the sanity check on a null pointer also goes away.
>
>> If we want to maintain better separate between VMX and Hyper-V code, then just make
>> the helper non-inline in hyperv.c, modifying MSR bitmaps will never be a hot path
>> for any sane guest.
>> 
>> I don't think I have a strong preference either way.  In a perfect world we'd keep
>> Hyper-V code separate, but practically speaking I think trying to move everything
>> into hyperv.c would result in far too many stubs and some weird function names.
>> 
>> Side topic, we should really have a wrapper for static_branch_unlikely(&enable_evmcs)
>> so that the static key can be defined iff CONFIG_HYPERV=y.  I'll send a patch.
>
> I.e.
>
> ---
>  arch/x86/kvm/vmx/hyperv.h | 11 -----------
>  arch/x86/kvm/vmx/vmx.c    |  9 +++++++--
>  2 files changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/hyperv.h b/arch/x86/kvm/vmx/hyperv.h
> index ab08a9b9ab7d..bac614e40078 100644
> --- a/arch/x86/kvm/vmx/hyperv.h
> +++ b/arch/x86/kvm/vmx/hyperv.h
> @@ -250,16 +250,6 @@ static inline u16 evmcs_read16(unsigned long field)
>  	return *(u16 *)((char *)current_evmcs + offset);
>  }
>  
> -static inline void evmcs_touch_msr_bitmap(void)
> -{
> -	if (unlikely(!current_evmcs))
> -		return;
> -
> -	if (current_evmcs->hv_enlightenments_control.msr_bitmap)
> -		current_evmcs->hv_clean_fields &=
> -			~HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP;
> -}
> -
>  static inline void evmcs_load(u64 phys_addr)
>  {
>  	struct hv_vp_assist_page *vp_ap =
> @@ -280,7 +270,6 @@ 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) {}
>  #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..ed4051b54412 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -3936,8 +3936,13 @@ static void vmx_msr_bitmap_l01_changed(struct vcpu_vmx *vmx)
>  	 * 'Enlightened MSR Bitmap' feature L0 needs to know that MSR
>  	 * bitmap has changed.
>  	 */
> -	if (static_branch_unlikely(&enable_evmcs))
> -		evmcs_touch_msr_bitmap();
> +	if (IS_ENABLED(CONFIG_HYPERV) && static_branch_unlikely(&enable_evmcs)) {
> +		struct hv_enlightened_vmcs *evmcs = (void *)vmx->vmcs01.vmcs;
> +
> +		if (evmcs->hv_enlightenments_control.msr_bitmap)
> +			evmcs->hv_clean_fields &=
> +				~HV_VMX_ENLIGHTENED_CLEAN_FIELD_MSR_BITMAP;
> +	}

With only one evmcs_touch_msr_bitmap() user in the tree, I think such
mixing of VMX and eVMCS is fine but honestly I like Alexandru's v3 more
as the "struct hv_enlightened_vmcs *" cast is the only thing which
"leaks" into VMX. Either way, no big deal)

>  
>  	vmx->nested.force_msr_bitmap_recalc = true;
>  }
>
> base-commit: 68bfbbf518a25856c2a3f07ea9d0c626f1b001fb

-- 
Vitaly


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-01-23 16:58 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-23 12:46 [PATCH v2] KVM: VMX: Fix crash due to uninitialized current_vmcs Alexandru Matei
2023-01-23 13:24 ` Vitaly Kuznetsov
2023-01-23 14:11   ` Alexandru Matei
2023-01-23 15:57     ` Sean Christopherson
2023-01-23 16:01       ` Sean Christopherson
2023-01-23 16:57         ` Vitaly Kuznetsov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.