get_writable_page verify flags breaks due to hugepage coalescing

[chc4]

Occasionally, the mmap syscall will crash with:

CR0: 0x80050033  CR3: 0xA000
CR2: 0x0  CR4: 0x350E20
RAX: 0x21403000  RBX: 0x0  RCX: 0x1006338
RDX: 0x3  RSI: 0x3F000  RDI: 0x0
RIP: 0x21AA  RBP: 0x1198A10  RSP: 0x11989E8
SS: 0x10  CS: 0x8  DS: 0x23  FS: 0x0  GS: 0x0
FS BASE: 0x0  GS BASE: 0x6050
Machine exception: page_at: pt entry not user writable  Data: 0x21403000
terminate called after throwing an instance of 'tinykvm::MemoryException'
  what():  page_at: pt entry not user writable

with a callstack pointing to

tinykvm/lib/tinykvm/machine_utils.cpp

Line 33 in fe757a7

auto* page = memory.get_writable_page(addr & ~PageMask(), memory.expectedUsermodeFlags(), true, false);

        collect_state_guest = master_vm.mmap_allocate(0x1000, 0x7, false);
        tinykvm::page_at(master_vm.main_memory(), collect_state_guest, [] (uint64_t addr, uint64_t& entry, size_t size) {
            // Make the page executable by the user (There is probably a better way to do this?)
            entry = entry & ~PDE64_NX | PDE64_DIRTY;
        });

        // Emulate the relevant mmap
        auto new_page = master_vm.mmap_allocate(258048, 3);
        master_vm.memzero(new_page, 258048);

is a reduced reproducer, although is a symptom of the issue showing up from a userspace program executing mmap(0x0, 258048, prot=3, flags=22, vfd=-1) = 0x21403000 instead. The collect_state page is an executable memory page that I'm allocating from the VMM - the issue "goes away" if you don't set PDE64_DIRTY in page_at, however removing all of the PDE64_DIRTY flags from the original program still causes a crash in a (slightly later) mmap call instead.

I believe the issue is due to the above page_at resolving to a hugepage that the newly mmap'd region is embedded within, and so it sees that collect_state_guest has the dirty bit set and thus must_be_zeroed = true, but then the later get_writable_page gets the hugepage which has PDE64_NX cleared and fails the flag against vMemory::expectedUsermodeFlags

This is maybe a case of me holding tinykvm wrong, and executable pages should somehow be allocated separately from non-executable pages? But mmap_allocate throws away prot, and so I'm not sure how else I'm supposed to allocate code pages from the VMM. It seems like the executable_heap MachineOption configures !NX everywhere via the vMemory::expectedUsermodeFlags, and so would cause or hide this issue depending on e.g. if you have a dynamic ELF or not and turn it off - but even in the non-executable case it seems like you could get unlucky and have the initial machine mapped .text page for your ELF coalesce with the first user serviced mmap and get sad.

[fwsGonzo]

I haven't looked at this yet, but I will attempt to explain how TinyKVM was originally designed.

TinyKVM sets up all pagetables at init-time and mandates that nothing changes, except for turning writable pages into cloneable, and then later copy-on-write can turn them back into writable. This has been changed with the addition of mmap-backed-files (only in an attempt to reduce RSS). The mmap-backed-files feature is just as bug-ridden as what you just experienced. I've commented out the protection changes because something goes awry somewhere, and I haven't figured out yet what.

My typical way of dealing with this has been to (for example) when loading static Deno, it puts JIT segments in a high memory location, which works well with remappings. A remapping is a heap allocation that steals pages from the heap, moves them to some virtual base, and then applies the given protections. Maybe that could work for you.

An example of this can be found here: https://github.com/libriscv/drogon_tinykvm/blob/master/tenants.json#L54-L61

[fwsGonzo]

Like you wrote, executable heap does hide the issue by making everything executable, and the ELF loader uses "heap_flags" to determine if the tail end of the ELF data section should start executable, which is where the heap would begin. We use this to load dynamic ELFs. We don't fully trust the memory layout yet though, as I have had (for example) problems loading Node.js.

TinyKVM has been slowly going in the direction of per-request isolation, where there is a master VM with a pristine state loaded, and if guest uses JIT there is a warmup. My idea was to go over the page tables once after initialization to apply the proper protections. Maybe that would help in your case too, if you are relying on a reset to a state where the mmap()'s have already happened. The original reason behind not doing any pagetable changes after ELF loading is because of attack surface. At load time I can see the page tables using vm.print_pagetables() and we can all agree on its soundness, etc.

With the "keep-working memory"-resets, which is current the state-of-the-art in VM reset performance, it's no longer desirable to make changes to forks that require a CR3 reload after reset. (We are faster to reset than WASM, and anyone else that we know of as a result!) That said, we should definitely aim to actually set the protections passed to mmap on the master VM before it's made forkable. It's just a matter of how: Either do it as they happen, or afterwards as a pass. mmap() is weird in that it's allocated in long permission-less atomic ranges which only afterwards receive the real protections in second and third calls etc.

[chc4]

"mandates that nothing change" also isn't exactly true, though, because it also does hugepage breaking at runtime or initializes zero pages on first trap right? It kind of seems to me that mmap_alloc should force a hugepage break if the permissions for page_at of the returned page are different than prot, because otherwise the hugepage coalescing here also means that a mmap with PROT_READ would potentially actually return a page offset inside a PROT_READ|PROT_WRITE hugepage.

Actually, looking at it now, it doesn't look like tinykvm would ever return a PROT_READ mapping since it treats the entire heap as RW or RWX...which also seems like a fairly big issue.

In response to your second comment,

Maybe that would help in your case too, if you are relying on a reset to a state where the mmap()'s have already happened

would probably resolve the initial collector_state and trampoline executable pages since I could allocate those pages upfront before running the VM instead of lazily, but I'd also like to be able to e.g. twiddle the NX bits for dynamic code pages at runtime in order to catch the first time code is executed out of them, which I think this still would have an issue with. I'm doing with this a fork specific patch to add a set_page_fault_callback, so it's perfectly fine to say no warrenty is provided and I'm on my own for that part however :)