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

[Sean Christopherson <seanjc@google.com>]

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
--