RE: hv_hypercall_pg page permissios

[Vitaly Kuznetsov]

Dexuan Cui <decui@xxxxxxxxxxxxx> writes:

>> From: linux-hyperv-owner@xxxxxxxxxxxxxxx
>> <linux-hyperv-owner@xxxxxxxxxxxxxxx> On Behalf Of Dexuan Cui
>> Sent: Monday, June 15, 2020 10:42 AM
>> > >
>> > > Hi hch,
>> > > The patch is merged into the mainine recently, but unluckily we noticed
>> > > a warning with CONFIG_DEBUG_WX=y
>> > >
>> > > Should we revert this patch, or figure out a way to ask the DEBUG_WX
>> > > code to ignore this page?
>> >
>> > Are you sure it is hv_hypercall_pg?
>> Yes, 100% sure. I printed the value of hv_hypercall_pg and and it matched the
>> address in the warning line " x86/mm: Found insecure W+X mapping at
>> address".
>
> I did this experiment:
>   1. export vmalloc_exec and ptdump_walk_pgd_level_checkwx.
>   2. write a test module that calls them.
>   3. It turns out that every call of vmalloc_exec() triggers such a warning.
>
> vmalloc_exec() uses PAGE_KERNEL_EXEC, which is defined as
>    (__PP|__RW|   0|___A|   0|___D|   0|___G)
>
> It looks the logic in note_page() is: for_each_RW_page, if the NX bit is unset,
> then report the page as an insecure W+X mapping. IMO this explains the
> warning?

Yea, bummer.

it seems we need something like PAGE_KERNEL_READONLY_EXEC but we don't
seem to have one on x86. Hypercall page is special in a way that the
guest doesn't need to write there at all. vmalloc_exec() seems to have
only one other user on x86: module_alloc() and it has other needs. On
ARM, alloc_insn_page() does the following:

arch/arm64/kernel/probes/kprobes.c:     page = vmalloc_exec(PAGE_SIZE);
arch/arm64/kernel/probes/kprobes.c-     if (page) {
arch/arm64/kernel/probes/kprobes.c-             set_memory_ro((unsigned long)page, 1);
arch/arm64/kernel/probes/kprobes.c-             set_vm_flush_reset_perms(page);
arch/arm64/kernel/probes/kprobes.c-     }

What if we do the same? (almost untested):

diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
index e2137070386a..31aadfea589b 100644
--- a/arch/x86/hyperv/hv_init.c
+++ b/arch/x86/hyperv/hv_init.c
@@ -23,6 +23,7 @@
 #include <linux/kernel.h>
 #include <linux/cpuhotplug.h>
 #include <linux/syscore_ops.h>
+#include <linux/set_memory.h>
 #include <clocksource/hyperv_timer.h>
 
 void *hv_hypercall_pg;
@@ -383,6 +384,8 @@ void __init hyperv_init(void)
                wrmsrl(HV_X64_MSR_GUEST_OS_ID, 0);
                goto remove_cpuhp_state;
        }
+       set_memory_ro((unsigned long)hv_hypercall_pg, 1);
+       set_vm_flush_reset_perms(hv_hypercall_pg);
 
        rdmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
        hypercall_msr.enable = 1;

-- 
Vitaly