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/[email protected]/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
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: [email protected]
Cc: Vitaly Kuznetsov <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---
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
Sean Christopherson <[email protected]> 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: [email protected]
> Cc: Vitaly Kuznetsov <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> 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 <[email protected]>
--
Vitaly
On Fri, Nov 05, 2021 at 11:16:14AM +0100, Vitaly Kuznetsov wrote:
> Sean Christopherson <[email protected]> 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: [email protected]
> > Cc: Vitaly Kuznetsov <[email protected]>
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > 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 <[email protected]>
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
>