linux-kernel - Re: [PATCH 2/2] hyperv: Do not overlap the input and output hypercall areas in get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8da58247-87df-4250-820a-758ea8e00bbb@linux.microsoft.com>
Date: Thu, 19 Dec 2024 10:19:07 -0800
From: Roman Kisel <romank@...ux.microsoft.com>
To: Wei Liu <wei.liu@...nel.org>
Cc: hpa@...or.com, kys@...rosoft.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, decui@...rosoft.com,
 eahariha@...ux.microsoft.com, haiyangz@...rosoft.com, mingo@...hat.com,
 mhklinux@...look.com, nunodasneves@...ux.microsoft.com, tglx@...utronix.de,
 tiala@...rosoft.com, linux-hyperv@...r.kernel.org,
 linux-kernel@...r.kernel.org, x86@...nel.org, apais@...rosoft.com,
 benhill@...rosoft.com, ssengar@...rosoft.com, sunilmut@...rosoft.com,
 vdso@...bites.dev
Subject: Re: [PATCH 2/2] hyperv: Do not overlap the input and output hypercall
 areas in get_vtl(void)



On 12/18/2024 6:42 PM, Wei Liu wrote:
> On Wed, Dec 18, 2024 at 12:54:21PM -0800, Roman Kisel wrote:
>> The Top-Level Functional Specification for Hyper-V, Section 3.6 [1, 2], disallows
>> overlapping of the input and output hypercall areas, and get_vtl(void) does
>> overlap them.
>>
>> To fix this, enable allocation of the output hypercall pages when running in
>> the VTL mode and use the output hypercall page of the current vCPU for the
>> hypercall.
>>
>> [1] https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/tlfs/hypercall-interface
>> [2] https://github.com/MicrosoftDocs/Virtualization-Documentation/tree/main/tlfs
>>
>> Fixes: 8387ce06d70b ("x86/hyperv: Set Virtual Trust Level in VMBus init message")
>> Signed-off-by: Roman Kisel <romank@...ux.microsoft.com>
>> ---
>>   arch/x86/hyperv/hv_init.c | 2 +-
>>   drivers/hv/hv_common.c    | 6 +++---
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
>> index c7185c6a290b..90c9ea00273e 100644
>> --- a/arch/x86/hyperv/hv_init.c
>> +++ b/arch/x86/hyperv/hv_init.c
>> @@ -422,7 +422,7 @@ static u8 __init get_vtl(void)
>>   
>>   	local_irq_save(flags);
>>   	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
>> -	output = (struct hv_get_vp_registers_output *)input;
>> +	output = *this_cpu_ptr(hyperv_pcpu_output_arg);
> 
> You can do
> 
> 	output = (char *)input + HV_HYP_PAGE_SIZE / 2;
> 
> to avoid the extra allocation.
> 
> The input and output structures surely won't take up half of the page.
Agreed on the both counts! I do think that the attempt to save here
won't help much: the hypercall output per-CPU pages in the VTL mode are
needed just as in the dom0/root partition mode because this hypercall
isn't going to be the only one required.

In other words, we will have to allocate these pages anyway as we evolve
the code; we are trying to save here what is going to be spent anyway. 
Sort of, kicking the can down the road as the saying goes :)

I do understand that within the code that is already merged, there is
just one this place in this function where the hypercall that returns
data is used. And the proposed approach makes the code self-explanatory:
```
output = *this_cpu_ptr(hyperv_pcpu_output_arg);
```

as opposed to

```
output = (char *)input + HV_HYP_PAGE_SIZE / 2;
```

or, as it existed,

```
output = (struct hv_get_vp_registers_output *)input;
```

which both do require a good comment I believe.

There will surely be more hypercall usage in the VTL mode that return
data and require the output pages as we progress with upstreaming the
VTL patches. Enabling the hypercall output pages allows to fix the
function in question in a very natural way, making it possible to
replace with some future `hv_get_vp_register` that would work for both
dom0 and VTL mode just the same.

All told, if you believe that we should make this patch a one-liner, 
I'll do as you suggested.

> 
> Thanks,
> Wei.
Thank you,
Roman

> 
>>   
>>   	memset(input, 0, struct_size(input, names, 1));
>>   	input->partition_id = HV_PARTITION_ID_SELF;
>> diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
>> index c4fd07d9bf1a..5178beed6ca8 100644
>> --- a/drivers/hv/hv_common.c
>> +++ b/drivers/hv/hv_common.c
>> @@ -340,7 +340,7 @@ int __init hv_common_init(void)
>>   	BUG_ON(!hyperv_pcpu_input_arg);
>>   
>>   	/* Allocate the per-CPU state for output arg for root */
>> -	if (hv_root_partition) {
>> +	if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
>>   		hyperv_pcpu_output_arg = alloc_percpu(void *);
>>   		BUG_ON(!hyperv_pcpu_output_arg);
>>   	}
>> @@ -435,7 +435,7 @@ int hv_common_cpu_init(unsigned int cpu)
>>   	void **inputarg, **outputarg;
>>   	u64 msr_vp_index;
>>   	gfp_t flags;
>> -	int pgcount = hv_root_partition ? 2 : 1;
>> +	const int pgcount = (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) ? 2 : 1;
>>   	void *mem;
>>   	int ret;
>>   
>> @@ -453,7 +453,7 @@ int hv_common_cpu_init(unsigned int cpu)
>>   		if (!mem)
>>   			return -ENOMEM;
>>   
>> -		if (hv_root_partition) {
>> +		if (hv_root_partition || IS_ENABLED(CONFIG_HYPERV_VTL_MODE)) {
>>   			outputarg = (void **)this_cpu_ptr(hyperv_pcpu_output_arg);
>>   			*outputarg = (char *)mem + HV_HYP_PAGE_SIZE;
>>   		}
>> -- 
>> 2.34.1
>>

-- 
Thank you,
Roman