[PATCH v2 0/2] x86/hyperv: Bug fix and enhancement

[PATCH v2 0/2] x86/hyperv: Bug fix and enhancement

[Sean Christopherson]

Patch 01 is a fix for a NULL pointer deref that I ran into with a bad VMM
configuration.  That specific error path is remedied by patch 02, but
Hyper-V can still end up in an inactive state if a memory allocation fails.

Patch 02 effectively makes the required MSRs mandatory for recognizing
Hyper-V at all.

Some versions of QEMU prior to ~6.0 make it all too easy to advertise
Hyper-V and a slew of features without advertising the Hyper-V HYPERCALL
MSR, e.g. +hv-ipi,+hv-tlbflush,+hv-vpindex,+hv-reenlightenment advertises
a bunch of things, but not the HYPERCALL MSR.

That results in the guest identifying Hyper-V and setting a variety of PV
ops that then get ignored because hyperv_init() silently disables Hyper-V
for all intents and purposes.  The VMM (or its controller) is obviously
off in the weeds, but ideally the guest kernel would acknowledge the bad
setup in some way.

v2:
  - Add Vitaly's review.
  - Rebase to hyperv-next, commit 285f68afa8b2 ("x86/hyperv: Protect
    set_hv_tscchange_cb() against getting preempted"). [Vitaly]
  - Tweak the changelog in patch 01 to omit the example about a bad VM
    config since the NULL check is needed even if that specific issue is
    resolved.

v1: https://lore.kernel.org/all/20211028222148.2924457-1-seanjc@google.com/t/#u

Sean Christopherson (2):
  x86/hyperv: Fix NULL deref in set_hv_tscchange_cb() if Hyper-V setup
    fails
  x86/hyperv: Move required MSRs check to initial platform probing

 arch/x86/hyperv/hv_init.c      | 12 ++++--------
 arch/x86/kernel/cpu/mshyperv.c | 20 +++++++++++++++-----
 2 files changed, 19 insertions(+), 13 deletions(-)

-- 
2.34.0.rc0.344.g81b53c2807-goog

[PATCH v2 1/2] x86/hyperv: Fix NULL deref in set_hv_tscchange_cb() if Hyper-V setup fails

[Sean Christopherson]

Check for a valid hv_vp_index array prior to derefencing hv_vp_index when
setting Hyper-V's TSC change callback.  If Hyper-V setup failed in
hyperv_init(), the kernel will still report that it's running under
Hyper-V, but will have silently disabled nearly all functionality.

  BUG: kernel NULL pointer dereference, address: 0000000000000010
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  PGD 0 P4D 0
  Oops: 0000 [#1] SMP
  CPU: 4 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc2+ #75
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
  RIP: 0010:set_hv_tscchange_cb+0x15/0xa0
  Code: <8b> 04 82 8b 15 12 17 85 01 48 c1 e0 20 48 0d ee 00 01 00 f6 c6 08
  ...
  Call Trace:
   kvm_arch_init+0x17c/0x280
   kvm_init+0x31/0x330
   vmx_init+0xba/0x13a
   do_one_initcall+0x41/0x1c0
   kernel_init_freeable+0x1f2/0x23b
   kernel_init+0x16/0x120
   ret_from_fork+0x22/0x30

Fixes: 93286261de1b ("x86/hyperv: Reenlightenment notifications support")
Cc: stable@vger.kernel.org
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/hyperv/hv_init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 24f4a06ac46a..7d252a58fbe4 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -177,6 +177,9 @@ void set_hv_tscchange_cb(void (*cb)(void))
 		return;
 	}
 
+	if (!hv_vp_index)
+		return;
+
 	hv_reenlightenment_cb = cb;
 
 	/* Make sure callback is registered before we write to MSRs */
-- 
2.34.0.rc0.344.g81b53c2807-goog

[PATCH v2 2/2] x86/hyperv: Move required MSRs check to initial platform probing

[Sean Christopherson]

Explicitly check for MSR_HYPERCALL and MSR_VP_INDEX support when probing
for running as a Hyper-V guest instead of waiting until hyperv_init() to
detect the bogus configuration.  Add messages to give the admin a heads
up that they are likely running on a broken virtual machine setup.

At best, silently disabling Hyper-V is confusing and difficult to debug,
e.g. the kernel _says_ it's using all these fancy Hyper-V features, but
always falls back to the native versions.  At worst, the half baked setup
will crash/hang the kernel.

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/hyperv/hv_init.c      |  9 +--------
 arch/x86/kernel/cpu/mshyperv.c | 20 +++++++++++++++-----
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 7d252a58fbe4..96eb7db31c8e 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -386,20 +386,13 @@ static void __init hv_get_partition_id(void)
  */
 void __init hyperv_init(void)
 {
-	u64 guest_id, required_msrs;
+	u64 guest_id;
 	union hv_x64_msr_hypercall_contents hypercall_msr;
 	int cpuhp;
 
 	if (x86_hyper_type != X86_HYPER_MS_HYPERV)
 		return;
 
-	/* Absolutely required MSRs */
-	required_msrs = HV_MSR_HYPERCALL_AVAILABLE |
-		HV_MSR_VP_INDEX_AVAILABLE;
-
-	if ((ms_hyperv.features & required_msrs) != required_msrs)
-		return;
-
 	if (hv_common_init())
 		return;
 
diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 4794b716ec79..ff55df60228f 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -163,12 +163,22 @@ static uint32_t  __init ms_hyperv_platform(void)
 	cpuid(HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS,
 	      &eax, &hyp_signature[0], &hyp_signature[1], &hyp_signature[2]);
 
-	if (eax >= HYPERV_CPUID_MIN &&
-	    eax <= HYPERV_CPUID_MAX &&
-	    !memcmp("Microsoft Hv", hyp_signature, 12))
-		return HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
+	if (eax < HYPERV_CPUID_MIN || eax > HYPERV_CPUID_MAX ||
+	    memcmp("Microsoft Hv", hyp_signature, 12))
+		return 0;
 
-	return 0;
+	/* HYPERCALL and VP_INDEX MSRs are mandatory for all features. */
+	eax = cpuid_eax(HYPERV_CPUID_FEATURES);
+	if (!(eax & HV_MSR_HYPERCALL_AVAILABLE)) {
+		pr_warn("x86/hyperv: HYPERCALL MSR not available.\n");
+		return 0;
+	}
+	if (!(eax & HV_MSR_VP_INDEX_AVAILABLE)) {
+		pr_warn("x86/hyperv: VP_INDEX MSR not available.\n");
+		return 0;
+	}
+
+	return HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
 }
 
 static unsigned char hv_get_nmi_reason(void)
-- 
2.34.0.rc0.344.g81b53c2807-goog

Re: [PATCH v2 1/2] x86/hyperv: Fix NULL deref in set_hv_tscchange_cb() if Hyper-V setup fails

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> Check for a valid hv_vp_index array prior to derefencing hv_vp_index when
> setting Hyper-V's TSC change callback.  If Hyper-V setup failed in
> hyperv_init(), the kernel will still report that it's running under
> Hyper-V, but will have silently disabled nearly all functionality.
>
>   BUG: kernel NULL pointer dereference, address: 0000000000000010
>   #PF: supervisor read access in kernel mode
>   #PF: error_code(0x0000) - not-present page
>   PGD 0 P4D 0
>   Oops: 0000 [#1] SMP
>   CPU: 4 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc2+ #75
>   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
>   RIP: 0010:set_hv_tscchange_cb+0x15/0xa0
>   Code: <8b> 04 82 8b 15 12 17 85 01 48 c1 e0 20 48 0d ee 00 01 00 f6 c6 08
>   ...
>   Call Trace:
>    kvm_arch_init+0x17c/0x280
>    kvm_init+0x31/0x330
>    vmx_init+0xba/0x13a
>    do_one_initcall+0x41/0x1c0
>    kernel_init_freeable+0x1f2/0x23b
>    kernel_init+0x16/0x120
>    ret_from_fork+0x22/0x30
>
> Fixes: 93286261de1b ("x86/hyperv: Reenlightenment notifications support")
> Cc: stable@vger.kernel.org
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/hyperv/hv_init.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 24f4a06ac46a..7d252a58fbe4 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -177,6 +177,9 @@ void set_hv_tscchange_cb(void (*cb)(void))
>  		return;
>  	}
>  
> +	if (!hv_vp_index)
> +		return;
> +

Arguably, we could've merged this with 'if (!hv_reenlightenment_available())' 
above to get a message printed:

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 24f4a06ac46a..4a2a091c2f0e 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -172,7 +172,7 @@ void set_hv_tscchange_cb(void (*cb)(void))
        };
        struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
 
-       if (!hv_reenlightenment_available()) {
+       if (!hv_reenlightenment_available() || !hv_vp_index) {
                pr_warn("Hyper-V: reenlightenment support is unavailable\n");
                return;
        }

just to have an indication that something is off.

>  	hv_reenlightenment_cb = cb;
>  
>  	/* Make sure callback is registered before we write to MSRs */

With or without the change,

Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

-- 
Vitaly

Re: [PATCH v2 1/2] x86/hyperv: Fix NULL deref in set_hv_tscchange_cb() if Hyper-V setup fails

[Wei Liu]

On Fri, Nov 05, 2021 at 11:16:14AM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > Check for a valid hv_vp_index array prior to derefencing hv_vp_index when
> > setting Hyper-V's TSC change callback.  If Hyper-V setup failed in
> > hyperv_init(), the kernel will still report that it's running under
> > Hyper-V, but will have silently disabled nearly all functionality.
> >
> >   BUG: kernel NULL pointer dereference, address: 0000000000000010
> >   #PF: supervisor read access in kernel mode
> >   #PF: error_code(0x0000) - not-present page
> >   PGD 0 P4D 0
> >   Oops: 0000 [#1] SMP
> >   CPU: 4 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc2+ #75
> >   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> >   RIP: 0010:set_hv_tscchange_cb+0x15/0xa0
> >   Code: <8b> 04 82 8b 15 12 17 85 01 48 c1 e0 20 48 0d ee 00 01 00 f6 c6 08
> >   ...
> >   Call Trace:
> >    kvm_arch_init+0x17c/0x280
> >    kvm_init+0x31/0x330
> >    vmx_init+0xba/0x13a
> >    do_one_initcall+0x41/0x1c0
> >    kernel_init_freeable+0x1f2/0x23b
> >    kernel_init+0x16/0x120
> >    ret_from_fork+0x22/0x30
> >
> > Fixes: 93286261de1b ("x86/hyperv: Reenlightenment notifications support")
> > Cc: stable@vger.kernel.org
> > Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/hyperv/hv_init.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> > index 24f4a06ac46a..7d252a58fbe4 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -177,6 +177,9 @@ void set_hv_tscchange_cb(void (*cb)(void))
> >  		return;
> >  	}
> >  
> > +	if (!hv_vp_index)
> > +		return;
> > +
> 
> Arguably, we could've merged this with 'if (!hv_reenlightenment_available())' 
> above to get a message printed:
> 
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 24f4a06ac46a..4a2a091c2f0e 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -172,7 +172,7 @@ void set_hv_tscchange_cb(void (*cb)(void))
>         };
>         struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
>  
> -       if (!hv_reenlightenment_available()) {
> +       if (!hv_reenlightenment_available() || !hv_vp_index) {
>                 pr_warn("Hyper-V: reenlightenment support is unavailable\n");
>                 return;
>         }
> 
> just to have an indication that something is off.
> 
> >  	hv_reenlightenment_cb = cb;
> >  
> >  	/* Make sure callback is registered before we write to MSRs */
> 
> With or without the change,
> 
> Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>

Thanks Sean and Vitaly. Both patches look good to me. They will be taken
via hyperv-fixes after v5.16-rc1 is cut.

Wei.

> 
> -- 
> Vitaly
>