Re: [PATCH] x86/hyperv: use dynamically allocated page for hypercalls

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/04/2025 5:51 pm, Ariadne Conill wrote:
> diff --git a/xen/arch/x86/guest/hyperv/hyperv.c 
> b/xen/arch/x86/guest/hyperv/hyperv.c
> index 6989af38f1..637b4bf335 100644
> --- a/xen/arch/x86/guest/hyperv/hyperv.c
> +++ b/xen/arch/x86/guest/hyperv/hyperv.c
> @@ -98,10 +97,22 @@ static void __init setup_hypercall_page(void)
>      rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>      if ( !hypercall_msr.enable )
>      {
> -        mfn = HV_HCALL_MFN;
> +        hv_hcall_page = alloc_xenheap_page();
> +        if ( !hv_hcall_page )
> +        {
> +            printk("Hyper-V: Failed to allocate hypercall trampoline 
> page\n");
> +            return -ENOMEM;
> +        }
> +
> +        printk("Hyper-V: Allocated hypercall page @ %p.\n", hv_hcall_page);
> +
> +        mfn = virt_to_mfn(hv_hcall_page);

Up to here is ok; this is just choosing a different page, but...

>          hypercall_msr.enable = 1;
>          hypercall_msr.guest_physical_address = mfn;
>          wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> +
> +        start = (unsigned long) hv_hcall_page;
> +        modify_xen_mappings(start, start + PAGE_SIZE, PAGE_HYPERVISOR_RX);

... this and ...

>      }
>      else
>          mfn = hypercall_msr.guest_physical_address;
> @@ -109,9 +120,9 @@ static void __init setup_hypercall_page(void)
>      rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
>      BUG_ON(!hypercall_msr.enable);
>  
> -    set_fixmap_x(FIX_X_HYPERV_HCALL, mfn << PAGE_SHIFT);

... this break the case where the overlay is already chosen and cannot move.

It really needs to stay using set_fixmap_x(), which in turn means you
can ...

> diff --git a/xen/arch/x86/include/asm/guest/hyperv-hcall.h 
> b/xen/arch/x86/include/asm/guest/hyperv-hcall.h
> index b76dbf9ccc..b73edca7c6 100644
> --- a/xen/arch/x86/include/asm/guest/hyperv-hcall.h
> +++ b/xen/arch/x86/include/asm/guest/hyperv-hcall.h
> @@ -20,13 +20,13 @@ static inline uint64_t hv_do_hypercall(uint64_t control, 
> paddr_t input_addr,
>                                         paddr_t output_addr)
>  {
>      uint64_t status;
> -    register unsigned long r8 asm ( "r8" ) = output_addr;
>  
>      /* See TLFS for volatile registers */
> -    asm volatile ( "call hv_hcall_page"
> +    asm volatile ( "mov %[output_addr], %%r8\n"
> +                   "call *%[target_addr]"
>                     : "=a" (status), "+c" (control),
>                       "+d" (input_addr) ASM_CALL_CONSTRAINT
> -                   : "r" (r8)
> +                   : [output_addr] "r" (output_addr), [target_addr] "r" 
> (hv_hcall_page)
>                     : "memory" );

... undo this speculative security vulnerability you've got by not using
INDIRECT_CALL.

The point of FIXMAP_X is to provide a virtual address in the main 1G
range for .text/.data/.rodata/.bss, which can point to an arbitrary
location, and can be regularly CALL'd.

~Andrew