[PATCH] x86/hyperv/vtl: Replace real_mode_header only under Hyper-V

[PATCH] x86/hyperv/vtl: Replace real_mode_header only under Hyper-V

[Mathias Krause]

Booting a CONFIG_HYPERV_VTL_MODE=y enabled kernel on bare metal or a
non-Hyper-V hypervisor leads to serve memory corruption as
hv_vtl_early_init() will run even though hv_vtl_init_platform() did not.
This skips no-oping the 'realmode_reserve' and 'realmode_init' platform
hooks, making init_real_mode() -> setup_real_mode() try to copy
'real_mode_blob' over 'real_mode_header' which we set to the stub
'hv_vtl_real_mode_header'. However, as 'real_mode_blob' isn't just a
'struct real_mode_header' -- it's the complete code! -- copying it over
'hv_vtl_real_mode_header' will corrupt quite some memory following it.

The real cause for this erroneous behaviour is that hv_vtl_early_init()
blindly assumes the kernel is running on Hyper-V, which it may not.

Fix this by making sure the code only replaces the real mode header with
the stub one iff the kernel is running under Hyper-V.

Fixes: 3be1bc2fe9d2 ("x86/hyperv: VTL support for Hyper-V")
Cc: Saurabh Sengar <ssengar@linux.microsoft.com>
Cc: stable@kernel.org
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 arch/x86/hyperv/hv_vtl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
index 57df7821d66c..54c06f4b8b4c 100644
--- a/arch/x86/hyperv/hv_vtl.c
+++ b/arch/x86/hyperv/hv_vtl.c
@@ -12,6 +12,7 @@
 #include <asm/desc.h>
 #include <asm/i8259.h>
 #include <asm/mshyperv.h>
+#include <asm/hypervisor.h>
 #include <asm/realmode.h>
 
 extern struct boot_params boot_params;
@@ -214,6 +215,9 @@ static int hv_vtl_wakeup_secondary_cpu(int apicid, unsigned long start_eip)
 
 static int __init hv_vtl_early_init(void)
 {
+	if (!hypervisor_is_type(X86_HYPER_MS_HYPERV))
+		return 0;
+
 	/*
 	 * `boot_cpu_has` returns the runtime feature support,
 	 * and here is the earliest it can be used.
-- 
2.30.2

Re: [PATCH] x86/hyperv/vtl: Replace real_mode_header only under Hyper-V

[Saurabh Singh Sengar]

On Fri, Sep 08, 2023 at 12:26:10PM +0200, Mathias Krause wrote:
> Booting a CONFIG_HYPERV_VTL_MODE=y enabled kernel on bare metal or a
> non-Hyper-V hypervisor leads to serve memory corruption as

FWIW, CONFIG_HYPERV_VTL_MODE is not expected to be enabled for non VTL
platforms. Referring Kconfig documentation:
"A kernel built with this option must run at VTL2, and will not run as
a normal guest."

- Saurabh

> hv_vtl_early_init() will run even though hv_vtl_init_platform() did not.
> This skips no-oping the 'realmode_reserve' and 'realmode_init' platform
> hooks, making init_real_mode() -> setup_real_mode() try to copy
> 'real_mode_blob' over 'real_mode_header' which we set to the stub
> 'hv_vtl_real_mode_header'. However, as 'real_mode_blob' isn't just a
> 'struct real_mode_header' -- it's the complete code! -- copying it over
> 'hv_vtl_real_mode_header' will corrupt quite some memory following it.
> 
> The real cause for this erroneous behaviour is that hv_vtl_early_init()
> blindly assumes the kernel is running on Hyper-V, which it may not.
> 
> Fix this by making sure the code only replaces the real mode header with
> the stub one iff the kernel is running under Hyper-V.
> 
> Fixes: 3be1bc2fe9d2 ("x86/hyperv: VTL support for Hyper-V")
> Cc: Saurabh Sengar <ssengar@linux.microsoft.com>
> Cc: stable@kernel.org
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---
>  arch/x86/hyperv/hv_vtl.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
> index 57df7821d66c..54c06f4b8b4c 100644
> --- a/arch/x86/hyperv/hv_vtl.c
> +++ b/arch/x86/hyperv/hv_vtl.c
> @@ -12,6 +12,7 @@
>  #include <asm/desc.h>
>  #include <asm/i8259.h>
>  #include <asm/mshyperv.h>
> +#include <asm/hypervisor.h>
>  #include <asm/realmode.h>
>  
>  extern struct boot_params boot_params;
> @@ -214,6 +215,9 @@ static int hv_vtl_wakeup_secondary_cpu(int apicid, unsigned long start_eip)
>  
>  static int __init hv_vtl_early_init(void)
>  {
> +	if (!hypervisor_is_type(X86_HYPER_MS_HYPERV))
> +		return 0;
> +
>  	/*
>  	 * `boot_cpu_has` returns the runtime feature support,
>  	 * and here is the earliest it can be used.
> -- 
> 2.30.2
> 

Re: [PATCH] x86/hyperv/vtl: Replace real_mode_header only under Hyper-V

[Mathias Krause]

On 08.09.23 17:02, Saurabh Singh Sengar wrote:
> On Fri, Sep 08, 2023 at 12:26:10PM +0200, Mathias Krause wrote:
>> Booting a CONFIG_HYPERV_VTL_MODE=y enabled kernel on bare metal or a
>> non-Hyper-V hypervisor leads to serve memory corruption as
> 
> FWIW, CONFIG_HYPERV_VTL_MODE is not expected to be enabled for non VTL
> platforms.

Fair enough, but there's really no excuse to randomly crashing the
kernel if one forgot to RTFM like I did. The code should (and easily
can) handle such situations, especially if it's just a matter of a two
line change.

> Referring Kconfig documentation:
> "A kernel built with this option must run at VTL2, and will not run as
> a normal guest."

So, maybe, the 'return 0' below should be a 'panic("Need to run on
Hyper-V!")' instead?

But then, looking at the code, most of the VTL specifics only run when
the Hyper-V hypervisor was actually detected during early boot. It's
just hv_vtl_early_init() that runs unconditionally and spoils the game.

Is there really a *hard* requirement / reason for having VTL support
disabled when not running under Hyper-V? At least I can't find any from
the code side. It'll all be fine with the below patch, also enabling
running the same kernel on multiple platforms -- bare metal, KVM, Hyper-V,..

Thanks,
Mathias

> 
> - Saurabh
> 
>> hv_vtl_early_init() will run even though hv_vtl_init_platform() did not.
>> This skips no-oping the 'realmode_reserve' and 'realmode_init' platform
>> hooks, making init_real_mode() -> setup_real_mode() try to copy
>> 'real_mode_blob' over 'real_mode_header' which we set to the stub
>> 'hv_vtl_real_mode_header'. However, as 'real_mode_blob' isn't just a
>> 'struct real_mode_header' -- it's the complete code! -- copying it over
>> 'hv_vtl_real_mode_header' will corrupt quite some memory following it.
>>
>> The real cause for this erroneous behaviour is that hv_vtl_early_init()
>> blindly assumes the kernel is running on Hyper-V, which it may not.
>>
>> Fix this by making sure the code only replaces the real mode header with
>> the stub one iff the kernel is running under Hyper-V.
>>
>> Fixes: 3be1bc2fe9d2 ("x86/hyperv: VTL support for Hyper-V")
>> Cc: Saurabh Sengar <ssengar@linux.microsoft.com>
>> Cc: stable@kernel.org
>> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
>> ---
>>  arch/x86/hyperv/hv_vtl.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/hyperv/hv_vtl.c b/arch/x86/hyperv/hv_vtl.c
>> index 57df7821d66c..54c06f4b8b4c 100644
>> --- a/arch/x86/hyperv/hv_vtl.c
>> +++ b/arch/x86/hyperv/hv_vtl.c
>> @@ -12,6 +12,7 @@
>>  #include <asm/desc.h>
>>  #include <asm/i8259.h>
>>  #include <asm/mshyperv.h>
>> +#include <asm/hypervisor.h>
>>  #include <asm/realmode.h>
>>  
>>  extern struct boot_params boot_params;
>> @@ -214,6 +215,9 @@ static int hv_vtl_wakeup_secondary_cpu(int apicid, unsigned long start_eip)
>>  
>>  static int __init hv_vtl_early_init(void)
>>  {
>> +	if (!hypervisor_is_type(X86_HYPER_MS_HYPERV))
>> +		return 0;
>> +
>>  	/*
>>  	 * `boot_cpu_has` returns the runtime feature support,
>>  	 * and here is the earliest it can be used.
>> -- 
>> 2.30.2
>>

Re: [PATCH] x86/hyperv/vtl: Replace real_mode_header only under Hyper-V

[Saurabh Singh Sengar]

On Mon, Sep 11, 2023 at 10:00:59AM +0200, Mathias Krause wrote:
> On 08.09.23 17:02, Saurabh Singh Sengar wrote:
> > On Fri, Sep 08, 2023 at 12:26:10PM +0200, Mathias Krause wrote:
> >> Booting a CONFIG_HYPERV_VTL_MODE=y enabled kernel on bare metal or a
> >> non-Hyper-V hypervisor leads to serve memory corruption as
> > 
> > FWIW, CONFIG_HYPERV_VTL_MODE is not expected to be enabled for non VTL
> > platforms.
> 
> Fair enough, but there's really no excuse to randomly crashing the
> kernel if one forgot to RTFM like I did. The code should (and easily
> can) handle such situations, especially if it's just a matter of a two
> line change.

Thanks, I understand your concern. We don't want people to enable this
flag by mistake and see unexpected behaviours.

To add extra safety for this flag, we can make this flag dependent on
EXPERT config.

- Saurabh

Re: [PATCH] x86/hyperv/vtl: Replace real_mode_header only under Hyper-V

[Mathias Krause]

On 13.09.23 07:27, Saurabh Singh Sengar wrote:
> On Mon, Sep 11, 2023 at 10:00:59AM +0200, Mathias Krause wrote:
>> On 08.09.23 17:02, Saurabh Singh Sengar wrote:
>>> On Fri, Sep 08, 2023 at 12:26:10PM +0200, Mathias Krause wrote:
>>>> Booting a CONFIG_HYPERV_VTL_MODE=y enabled kernel on bare metal or a
>>>> non-Hyper-V hypervisor leads to serve memory corruption as
>>>
>>> FWIW, CONFIG_HYPERV_VTL_MODE is not expected to be enabled for non VTL
>>> platforms.
>>
>> Fair enough, but there's really no excuse to randomly crashing the
>> kernel if one forgot to RTFM like I did. The code should (and easily
>> can) handle such situations, especially if it's just a matter of a two
>> line change.
> 
> Thanks, I understand your concern. We don't want people to enable this
> flag by mistake and see unexpected behaviours.

Unexpected behaviour like randomly crashing the kernel? ;)

> To add extra safety for this flag, we can make this flag dependent on
> EXPERT config.

Well, if you want to prevent people from using it, make it depend on
BROKEN, because that's what it is. All the other hypervisor support in
the kernel (Xen, VMware, KVM, ACRN, Jailhouse, even plain Hyper-V) can
perfectly cope with getting booted on a different hypervisor or bare
metal. Why is Hyper-V's VTL mode such a special snow flake that it has
to cause random memory corruption and, in turn, crash the kernel with
spectacular (and undebugable) fireworks if it's not booted under Hyper-V?

Thanks,
Mathias

Re: [PATCH] x86/hyperv/vtl: Replace real_mode_header only under Hyper-V

[Saurabh Singh Sengar]

On Fri, Sep 15, 2023 at 09:06:15AM +0200, Mathias Krause wrote:
> On 13.09.23 07:27, Saurabh Singh Sengar wrote:
> > On Mon, Sep 11, 2023 at 10:00:59AM +0200, Mathias Krause wrote:
> >> On 08.09.23 17:02, Saurabh Singh Sengar wrote:
> >>> On Fri, Sep 08, 2023 at 12:26:10PM +0200, Mathias Krause wrote:
> >>>> Booting a CONFIG_HYPERV_VTL_MODE=y enabled kernel on bare metal or a
> >>>> non-Hyper-V hypervisor leads to serve memory corruption as
> >>>
> >>> FWIW, CONFIG_HYPERV_VTL_MODE is not expected to be enabled for non VTL
> >>> platforms.

<snip>

> 
> Well, if you want to prevent people from using it, make it depend on
> BROKEN, because that's what it is. All the other hypervisor support in
> the kernel (Xen, VMware, KVM, ACRN, Jailhouse, even plain Hyper-V) can
> perfectly cope with getting booted on a different hypervisor or bare
> metal. Why is Hyper-V's VTL mode such a special snow flake that it has
> to cause random memory corruption and, in turn, crash the kernel with
> spectacular (and undebugable) fireworks if it's not booted under Hyper-V?

'BROKEN' is certainly not the right choice here. If it is used on the
correct platform as it is designed to be nothing is broken.

The default option for CONFIG_HYPERV_VTL_MODE is set to 'N', there is
sufficient documentation for it as well. I agree there can be cases where
people can still end up enabling it, for that EXPERT is a reasonable
solution.

- Saurabh



> 
> Thanks,
> Mathias

Re: [PATCH] x86/hyperv/vtl: Replace real_mode_header only under Hyper-V

[Mathias Krause]

On 15.09.23 13:32, Saurabh Singh Sengar wrote:
> On Fri, Sep 15, 2023 at 09:06:15AM +0200, Mathias Krause wrote:
>> On 13.09.23 07:27, Saurabh Singh Sengar wrote:
>>> On Mon, Sep 11, 2023 at 10:00:59AM +0200, Mathias Krause wrote:
>>>> On 08.09.23 17:02, Saurabh Singh Sengar wrote:
>>>>> On Fri, Sep 08, 2023 at 12:26:10PM +0200, Mathias Krause wrote:
>>>>>> Booting a CONFIG_HYPERV_VTL_MODE=y enabled kernel on bare metal or a
>>>>>> non-Hyper-V hypervisor leads to serve memory corruption as
>>>>>
>>>>> FWIW, CONFIG_HYPERV_VTL_MODE is not expected to be enabled for non VTL
>>>>> platforms.
> 
> <snip>
> 
>>
>> Well, if you want to prevent people from using it, make it depend on
>> BROKEN, because that's what it is. All the other hypervisor support in
>> the kernel (Xen, VMware, KVM, ACRN, Jailhouse, even plain Hyper-V) can
>> perfectly cope with getting booted on a different hypervisor or bare
>> metal. Why is Hyper-V's VTL mode such a special snow flake that it has
>> to cause random memory corruption and, in turn, crash the kernel with
>> spectacular (and undebugable) fireworks if it's not booted under Hyper-V?
> 
> 'BROKEN' is certainly not the right choice here. If it is used on the
> correct platform as it is designed to be nothing is broken.

This "if" is all I'm complaining about. If that assumption gets broken,
for whatever reason (a user wrongly enabling Kconfig options / a distro
trying to build a kernel that can run on many platforms / ...), the
kernel should still behave instead of corrupting memory leading to a
kernel crash -- especially if it's that dumb simple to handle this case
just fine.

So please, can you answer my question above, why VTL is such a special
snow flake that it needs no error handling nor validation of its core
assumptions like all other hypervisor code in the kernel seem to get right?

> 
> The default option for CONFIG_HYPERV_VTL_MODE is set to 'N', there is
> sufficient documentation for it as well. I agree there can be cases where
> people can still end up enabling it, for that EXPERT is a reasonable
> solution.

I don't see why this would solve anything, less so in preventing the
memory corruption angle which can easily be avoided.


Thanks,
Mathias