Patch 01 is a fix for a NULL pointer deref that I ran into with a bad VMM
configuration.
Patch 02 effectively makes the required MSRs mandatory for recognizing
Hyper-V at all. I'm not confident this is truly desirable, e.g. there
might be some features that are still kinda sorta usable, but on the other
hand there's a large pile of features that end up being a waste of cycles
to worm their way back to the native ops.
QEMU 5.1 (and other versions) makes it all too easy to advertise Hyper-V
and a slew of features without advertising the Hyper-V HYPERCALL MSR, e.g.
forcing QEMU features +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.
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 | 16 +++++++---------
arch/x86/kernel/cpu/mshyperv.c | 20 +++++++++++++++-----
2 files changed, 22 insertions(+), 14 deletions(-)
--
2.33.0.1079.g6e70778dc9-goog
Check for re-enlightenment support and 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(), e.g. because of a
bad VMM config that doesn't advertise the HYPERCALL MSR, 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 | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index 708a2712a516..6cc845c026d4 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -139,7 +139,7 @@ void set_hv_tscchange_cb(void (*cb)(void))
struct hv_reenlightenment_control re_ctrl = {
.vector = HYPERV_REENLIGHTENMENT_VECTOR,
.enabled = 1,
- .target_vp = hv_vp_index[smp_processor_id()]
+ .target_vp = -1,
};
struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
@@ -148,6 +148,11 @@ void set_hv_tscchange_cb(void (*cb)(void))
return;
}
+ if (!hv_vp_index)
+ return;
+
+ re_ctrl.target_vp = hv_vp_index[smp_processor_id()];
+
hv_reenlightenment_cb = cb;
/* Make sure callback is registered before we write to MSRs */
--
2.33.0.1079.g6e70778dc9-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.
Cc: 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 6cc845c026d4..abfb09610e22 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -347,20 +347,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 e095c28d27ae..ef6316fef99f 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.33.0.1079.g6e70778dc9-goog
Sean Christopherson <[email protected]> writes:
> Check for re-enlightenment support and 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(), e.g. because of a
> bad VMM config that doesn't advertise the HYPERCALL MSR, 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 | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 708a2712a516..6cc845c026d4 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -139,7 +139,7 @@ void set_hv_tscchange_cb(void (*cb)(void))
> struct hv_reenlightenment_control re_ctrl = {
> .vector = HYPERV_REENLIGHTENMENT_VECTOR,
> .enabled = 1,
> - .target_vp = hv_vp_index[smp_processor_id()]
> + .target_vp = -1,
> };
> struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
>
> @@ -148,6 +148,11 @@ void set_hv_tscchange_cb(void (*cb)(void))
> return;
> }
>
> + if (!hv_vp_index)
> + return;
> +
> + re_ctrl.target_vp = hv_vp_index[smp_processor_id()];
> +
> hv_reenlightenment_cb = cb;
>
> /* Make sure callback is registered before we write to MSRs */
The patch looks good, however, it needs to be applied on top of the
already merged:
https://lore.kernel.org/linux-hyperv/[email protected]/
--
Vitaly
Sean Christopherson <[email protected]> writes:
> 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.
>
> Cc: 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 6cc845c026d4..abfb09610e22 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -347,20 +347,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 e095c28d27ae..ef6316fef99f 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)
In theory, we can get away without VP_INDEX MSR as e.g. PV spinlocks
don't need it but it will require us to add the check to all other
features which actually need it and disable them so it's probably not
worth the effort (and unless we're running on KVM/QEMU which actually
*can* create such configuration, it's likely impossible to meet such
setup in real life).
Reviewed-by: Vitaly Kuznetsov <[email protected]>
--
Vitaly
On Fri, Oct 29, 2021 at 11:14:59AM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <[email protected]> writes:
>
> > Check for re-enlightenment support and 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(), e.g. because of a
> > bad VMM config that doesn't advertise the HYPERCALL MSR, 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 | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> > index 708a2712a516..6cc845c026d4 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -139,7 +139,7 @@ void set_hv_tscchange_cb(void (*cb)(void))
> > struct hv_reenlightenment_control re_ctrl = {
> > .vector = HYPERV_REENLIGHTENMENT_VECTOR,
> > .enabled = 1,
> > - .target_vp = hv_vp_index[smp_processor_id()]
> > + .target_vp = -1,
> > };
> > struct hv_tsc_emulation_control emu_ctrl = {.enabled = 1};
> >
> > @@ -148,6 +148,11 @@ void set_hv_tscchange_cb(void (*cb)(void))
> > return;
> > }
> >
> > + if (!hv_vp_index)
> > + return;
> > +
> > + re_ctrl.target_vp = hv_vp_index[smp_processor_id()];
> > +
> > hv_reenlightenment_cb = cb;
> >
> > /* Make sure callback is registered before we write to MSRs */
>
> The patch looks good, however, it needs to be applied on top of the
> already merged:
>
> https://lore.kernel.org/linux-hyperv/[email protected]/
Sean, are you going to rebase?
Wei.
On Tue, Nov 02, 2021, Wei Liu wrote:
> On Fri, Oct 29, 2021 at 11:14:59AM +0200, Vitaly Kuznetsov wrote:
> > Sean Christopherson <[email protected]> writes:
> > The patch looks good, however, it needs to be applied on top of the
> > already merged:
> >
> > https://lore.kernel.org/linux-hyperv/[email protected]/
>
> Sean, are you going to rebase?
Yep, I'll rebase.
On Tue, Nov 02, 2021 at 02:31:57PM +0000, Sean Christopherson wrote:
> On Tue, Nov 02, 2021, Wei Liu wrote:
> > On Fri, Oct 29, 2021 at 11:14:59AM +0200, Vitaly Kuznetsov wrote:
> > > Sean Christopherson <[email protected]> writes:
> > > The patch looks good, however, it needs to be applied on top of the
> > > already merged:
> > >
> > > https://lore.kernel.org/linux-hyperv/[email protected]/
> >
> > Sean, are you going to rebase?
>
> Yep, I'll rebase.
Thank you!
Wei.
On Fri, Oct 29, 2021 at 11:20:49AM +0200, Vitaly Kuznetsov wrote:
> Sean Christopherson <[email protected]> writes:
>
> > 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.
> >
> > Cc: 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 6cc845c026d4..abfb09610e22 100644
> > --- a/arch/x86/hyperv/hv_init.c
> > +++ b/arch/x86/hyperv/hv_init.c
> > @@ -347,20 +347,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 e095c28d27ae..ef6316fef99f 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)
>
> In theory, we can get away without VP_INDEX MSR as e.g. PV spinlocks
> don't need it but it will require us to add the check to all other
> features which actually need it and disable them so it's probably not
> worth the effort (and unless we're running on KVM/QEMU which actually
> *can* create such configuration, it's likely impossible to meet such
> setup in real life).
>
Indeed. There is no need to make things more complicated than necessary.
> Reviewed-by: Vitaly Kuznetsov <[email protected]>
Thanks for reviewing.
Wei.
>
> --
> Vitaly
>
On Thu, Nov 04, 2021 at 12:10:30PM +0000, Wei Liu wrote:
> On Tue, Nov 02, 2021 at 02:31:57PM +0000, Sean Christopherson wrote:
> > On Tue, Nov 02, 2021, Wei Liu wrote:
> > > On Fri, Oct 29, 2021 at 11:14:59AM +0200, Vitaly Kuznetsov wrote:
> > > > Sean Christopherson <[email protected]> writes:
> > > > The patch looks good, however, it needs to be applied on top of the
> > > > already merged:
> > > >
> > > > https://lore.kernel.org/linux-hyperv/[email protected]/
> > >
> > > Sean, are you going to rebase?
> >
> > Yep, I'll rebase.
>
> Thank you!
One further note: Vitaly's patch just hit mainline two days ago, but
v5.16-rc1 is not tagged yet. Feel free to base you new patches on
hyperv-next or linux-next. I will handle the rest. Or you can wait
until v5.16-rc1 is tagged and base your patches on that. Your call.
Thanks,
Wei.
>
> Wei.