From 9756ec6f462e10d9dea6f4191678814aa40402a6 Mon Sep 17 00:00:00 2001 From: Per Buer Date: Sun, 2 Aug 2026 09:28:04 +0200 Subject: [PATCH] tests: failing regression tests for the open audit findings Each test asserts the correct behaviour, so all of them fail on master and pass once the corresponding bug is fixed. Filed as a draft so the issues have something concrete to point at; not for merging before the fixes. syscalls.cpp dup2 onto a fresh vfd, poll with >256 valid fds, futex with an unimplemented op (#99 #102 #101) reset.cpp signal handlers, cwd and brk across reset_to (#103) remote.cpp remote call with a guest-zeroed FSBASE (#104) elfmal.cpp new file: five malformed-ELF cases covering resolve(), section_by_name(), resolve_symbol() and is_dynamic_elf() (#100) elfmal.cpp is registered in CMakeLists.txt. Four of its five cases crash the process rather than failing an assertion, so run them individually by name filter -- and note that a name filter skips the "Initialize KVM" case that calls Machine::init(), so pass it as well: ./elfmal "Initialize KVM,resolve must not trust*" Known-unrelated failures on master, present before these tests: the reset suite's "Execute function in VM (crash recovery)" case and two cases in the tegridy suite. Co-Authored-By: Claude Fable 5 --- tests/unit/CMakeLists.txt | 1 + tests/unit/elfmal.cpp | 300 ++++++++++++++++++++++++++++++++++++++ tests/unit/remote.cpp | 100 +++++++++++++ tests/unit/reset.cpp | 234 +++++++++++++++++++++++++++++ tests/unit/syscalls.cpp | 139 ++++++++++++++++++ 5 files changed, 774 insertions(+) create mode 100644 tests/unit/elfmal.cpp diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 6fd4fe6c..25182872 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -23,6 +23,7 @@ endfunction() if (TINYKVM_ARCH STREQUAL "AMD64") add_unit_test(basic basic.cpp) add_unit_test(elf elf.cpp) + add_unit_test(elfmal elfmal.cpp) add_unit_test(fork fork.cpp) add_unit_test(mmap mmap.cpp) add_unit_test(remote remote.cpp) diff --git a/tests/unit/elfmal.cpp b/tests/unit/elfmal.cpp new file mode 100644 index 00000000..ffe85d6f --- /dev/null +++ b/tests/unit/elfmal.cpp @@ -0,0 +1,300 @@ +#include + +#include +#include +#include +#include +#include +#include +#include + +/* Regression tests for malicious-ELF loader bugs in lib/tinykvm/machine_elf.cpp, + confirmed by the security audit (see hund.md). Each test crafts the hostile + ELF in memory -- a C++ port of the audit PoC craft scripts -- and drives the + same API path as the PoC harness (Machine::resolve, Machine::AddressOf, + is_dynamic_elf). Every test asserts the SAFE behavior: graceful rejection + (MachineException, or "symbol not found") instead of an out-of-bounds read + of host memory. + + On the unpatched tree every test here FAILS: the resolve() test fails its + size assertion, the others SIGSEGV (or abort under ASan) on the OOB read. + To make the OOB reads deterministic without sanitizers, each hostile ELF is + placed flush against a PROT_NONE guard page, so the first out-of-bounds + byte faults instead of silently consuming neighboring memory. A segfault + takes down the whole test process, so on an unpatched tree run the cases + individually by name, e.g.: ./elfmal "section_by_name*" + + Note: Machine::resolve() is also reachable from an unprivileged guest -- + vCPU::handle_exception() (vcpu_run.cpp) calls it to symbolicate a faulting + RIP -- so these bugs are triggerable by the guest, not just the embedder. */ + +namespace { + /* Struct writers mirroring the audit PoC craft scripts (System V ELF64, + little-endian, matching the host). */ + std::string elf_ehdr(uint16_t e_type, uint64_t e_entry, uint64_t e_phoff, + uint64_t e_shoff, uint16_t e_phnum, uint16_t e_shnum, uint16_t e_shstrndx) + { + Elf64_Ehdr h{}; + h.e_ident[EI_MAG0] = ELFMAG0; + h.e_ident[EI_MAG1] = ELFMAG1; + h.e_ident[EI_MAG2] = ELFMAG2; + h.e_ident[EI_MAG3] = ELFMAG3; + h.e_ident[EI_CLASS] = ELFCLASS64; + h.e_ident[EI_DATA] = ELFDATA2LSB; + h.e_ident[EI_VERSION] = EV_CURRENT; + h.e_ident[EI_OSABI] = ELFOSABI_NONE; + h.e_type = e_type; + h.e_machine = EM_X86_64; + h.e_version = EV_CURRENT; + h.e_entry = e_entry; + h.e_phoff = e_phoff; + h.e_shoff = e_shoff; + h.e_ehsize = sizeof(Elf64_Ehdr); + h.e_phentsize = sizeof(Elf64_Phdr); + h.e_phnum = e_phnum; + h.e_shentsize = sizeof(Elf64_Shdr); + h.e_shnum = e_shnum; + h.e_shstrndx = e_shstrndx; + return std::string(reinterpret_cast(&h), sizeof(h)); + } + std::string elf_phdr(uint32_t p_type, uint32_t p_flags, uint64_t p_offset, + uint64_t p_vaddr, uint64_t p_filesz, uint64_t p_memsz, uint64_t p_align = 0x1000) + { + Elf64_Phdr p{}; + p.p_type = p_type; + p.p_flags = p_flags; + p.p_offset = p_offset; + p.p_vaddr = p_vaddr; + p.p_paddr = p_vaddr; + p.p_filesz = p_filesz; + p.p_memsz = p_memsz; + p.p_align = p_align; + return std::string(reinterpret_cast(&p), sizeof(p)); + } + std::string elf_shdr(uint32_t sh_name, uint32_t sh_type, + uint64_t sh_offset, uint64_t sh_size, uint64_t sh_entsize = 0) + { + Elf64_Shdr s{}; + s.sh_name = sh_name; + s.sh_type = sh_type; + s.sh_offset = sh_offset; + s.sh_size = sh_size; + s.sh_addralign = 1; + s.sh_entsize = sh_entsize; + return std::string(reinterpret_cast(&s), sizeof(s)); + } + std::string elf_sym(uint32_t st_name, uint8_t st_info, + uint64_t st_value, uint64_t st_size) + { + Elf64_Sym s{}; + s.st_name = st_name; + s.st_info = st_info; + s.st_value = st_value; + s.st_size = st_size; + return std::string(reinterpret_cast(&s), sizeof(s)); + } + + const std::string SHSTRTAB("\0.shstrtab\0.symtab\0.strtab\0", 27); + constexpr uint32_t NM_SHSTRTAB = 1, NM_SYMTAB = 11, NM_STRTAB = 19; + /* mov byte [0], 0 ; jmp $ -- faults immediately if the guest is run. */ + const std::string FAULTING_CODE("\xC6\x04\x25\x00\x00\x00\x00\x00\xEB\xFE", 10); + constexpr uint64_t VADDR = 0x200000; + + /* cand1: loadable ELF whose .symtab holds one FUNC symbol with a + 10000-byte name, st_value=0 and st_size=~0, so resolve() takes the + direct-match branch for any small RIP. */ + std::string craft_longsym() + { + const std::string name(10000, 'S'); + const std::string strtab = std::string(1, '\0') + name + std::string(1, '\0'); + size_t cur = 64 + 56 + FAULTING_CODE.size(); + cur = (cur + 7) & ~size_t(7); + const size_t off_shdrs = cur; cur += 3 * 64; + const size_t off_shstrtab = cur; cur += SHSTRTAB.size(); + const size_t off_strtab = cur; cur += strtab.size(); + cur = (cur + 7) & ~size_t(7); + const size_t off_symtab = cur; cur += 24; + const size_t filesize = cur; + std::string d = elf_ehdr(ET_EXEC, VADDR + 64 + 56, 64, + off_shdrs, /*phnum=*/1, /*shnum=*/3, /*shstrndx=*/0); + d += elf_phdr(PT_LOAD, 5, 0, VADDR, filesize, filesize); /* R+X */ + d += FAULTING_CODE; + d.append(off_shdrs - d.size(), '\0'); + d += elf_shdr(NM_SHSTRTAB, SHT_STRTAB, off_shstrtab, SHSTRTAB.size()); + d += elf_shdr(NM_SYMTAB, SHT_SYMTAB, off_symtab, 24, 24); + d += elf_shdr(NM_STRTAB, SHT_STRTAB, off_strtab, strtab.size()); + d += SHSTRTAB; + d += strtab; + d.append(off_symtab - d.size(), '\0'); + d += elf_sym(1, 0x12, 0, ~0ULL); /* GLOBAL FUNC, covers all addresses */ + return d; + } + + /* cand2: e_shnum = 0xFFFF with a single real section header; the + section_by_name() loop walks 65535 headers past the end of the file. */ + std::string craft_shnum_oob() + { + std::string d = elf_ehdr(ET_EXEC, 0, 64, + /*shoff=*/64, /*phnum=*/0, /*shnum=*/0xFFFF, /*shstrndx=*/0); + d += elf_shdr(0, SHT_STRTAB, 0, 128); /* one shdr; loop continues past EOF */ + return d; + } + + /* Layout shared by cand3/cand3b: valid .shstrtab/.symtab/.strtab sections + with the symtab placed at the very end of the file. */ + std::string craft_symtab_at_eof(uint64_t symtab_sh_size, + const std::string& first_sym, const std::string& extra_sym) + { + const std::string strtab("\0AAAA\0", 6); + size_t cur = 64 + 3 * 64; + const size_t off_shstrtab = cur; cur += SHSTRTAB.size(); + const size_t off_strtab = cur; cur += strtab.size(); + cur = (cur + 7) & ~size_t(7); + const size_t off_symtab = cur; cur += 24 + extra_sym.size(); + std::string d = elf_ehdr(ET_EXEC, 0, 64, + /*shoff=*/64, /*phnum=*/0, /*shnum=*/3, /*shstrndx=*/0); + d += elf_shdr(NM_SHSTRTAB, SHT_STRTAB, off_shstrtab, SHSTRTAB.size()); + d += elf_shdr(NM_SYMTAB, SHT_SYMTAB, off_symtab, symtab_sh_size, 24); + d += elf_shdr(NM_STRTAB, SHT_STRTAB, off_strtab, strtab.size()); + d += SHSTRTAB; + d += strtab; + d.append(off_symtab - d.size(), '\0'); + d += first_sym; /* "AAAA", never matches the searched name */ + d += extra_sym; + return d; + } + + /* cand3: .symtab sh_size = 16MB but only one real entry at EOF; + resolve_symbol() walks entries far past the end of the file. */ + std::string craft_shsize_oob() + { + return craft_symtab_at_eof(0x1000000, elf_sym(1, 0x12, 0, 0), ""); + } + + /* cand3b: valid 2-entry .symtab; the second entry has st_name pointing + 16MB past .strtab -- a wild pointer for strcmp(). */ + std::string craft_stname_wild() + { + return craft_symtab_at_eof(48, elf_sym(1, 0x12, 0, 0), + elf_sym(0xFFFFF0, 0x12, 0, 0)); + } + + /* cand4: PT_INTERP with p_offset = -16, p_filesz = 32; the add-then-check + p_offset + p_filesz > size wraps to 16 and passes, after which + std::string(data + p_offset, p_filesz) reads before the buffer. */ + std::string craft_interp_wrap() + { + std::string d = elf_ehdr(ET_EXEC, 0, 64, + /*shoff=*/0, /*phnum=*/1, /*shnum=*/0, /*shstrndx=*/0); + d += elf_phdr(PT_INTERP, 4, 0xFFFFFFFFFFFFFFF0ULL, 0, 0x20, 0x20); + return d; + } + + /* Copies bytes into an mmap'd region with a PROT_NONE page immediately + before or after, so an out-of-bounds access faults deterministically + instead of reading whatever happens to neighbor a heap allocation. */ + struct GuardedView { + void* mapping = nullptr; + size_t mapping_len = 0; + std::string_view view; + + GuardedView(std::string_view bytes, bool guard_before) + { + constexpr size_t PAGE = 0x1000; + const size_t body = (bytes.size() + PAGE - 1) & ~(PAGE - 1); + mapping_len = body + PAGE; + mapping = mmap(nullptr, mapping_len, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); + REQUIRE(mapping != MAP_FAILED); + char* guard = guard_before ? (char*)mapping : (char*)mapping + body; + REQUIRE(mprotect(guard, PAGE, PROT_NONE) == 0); + char* dst = guard_before ? (char*)mapping + PAGE + : (char*)mapping + body - bytes.size(); + std::memcpy(dst, bytes.data(), bytes.size()); + view = std::string_view(dst, bytes.size()); + } + ~GuardedView() { if (mapping) munmap(mapping, mapping_len); } + GuardedView(const GuardedView&) = delete; + }; + + /* Safe behavior for the symbol-lookup bugs: either the loader rejects the + malformed section table, or the lookup simply finds nothing. What must + not happen is an out-of-bounds read (which the guard page turns into a + fatal signal on the unpatched tree). */ + void check_lookup_rejects(std::string_view binary) + { + bool rejected = false; + uint64_t addr = 1; + try { + addr = tinykvm::Machine::AddressOf("ZZZ_no_such", binary); + } catch (const tinykvm::MachineException&) { + rejected = true; + } + REQUIRE((rejected || addr == 0)); + } +} + +TEST_CASE("Initialize KVM", "[Initialize]") +{ + tinykvm::Machine::init(); +} + +TEST_CASE("resolve must not trust snprintf would-be length (cand1)", "[elfmal]") +{ + /* machine_elf.cpp resolve(): snprintf(result, 2048, "%s + 0x%lX", name, off) + returns the would-be length (~10008 for the crafted name), and + std::string(result, len) then reads ~8KB past the 2048-byte stack + buffer, disclosing host stack bytes to the caller (the guest, via the + exception printer path). Safe behavior: the returned string is bounded + by the buffer it was formatted into. */ + const std::string bin = craft_longsym(); + tinykvm::Machine::init(); /* also covers filtered runs without [Initialize] */ + tinykvm::MachineOptions opts{}; + opts.max_mem = 256ULL << 20; + tinykvm::Machine machine{std::string_view(bin), opts}; + const std::string resolved = machine.resolve(0x1000); + CAPTURE(resolved.size()); + REQUIRE(resolved.size() < 2048); +} + +TEST_CASE("section_by_name must validate e_shnum against binary size (cand2)", "[elfmal]") +{ + /* machine_elf.cpp section_by_name(): e_shnum/e_shstrndx/sh_name are used + without any check against the binary size. With e_shnum = 0xFFFF the + loop reads section headers past EOF and strcmp()s name pointers + computed from attacker-controlled bytes. */ + const std::string bin = craft_shnum_oob(); + const GuardedView gv(bin, /*guard_before=*/false); + check_lookup_rejects(gv.view); +} + +TEST_CASE("resolve_symbol must validate symtab sh_size against binary size (cand3)", "[elfmal]") +{ + /* machine_elf.cpp resolve_symbol(): symtab_ents = sh_size / sizeof(Sym) + is trusted, so a 16MB sh_size with a 1-entry symtab at EOF walks the + symbol table far past the end of the file. */ + const std::string bin = craft_shsize_oob(); + const GuardedView gv(bin, /*guard_before=*/false); + check_lookup_rejects(gv.view); +} + +TEST_CASE("resolve_symbol must validate st_name against strtab size (cand3b)", "[elfmal]") +{ + /* machine_elf.cpp resolve_symbol(): &strtab[sym.st_name] is handed to + strcmp() with no check that st_name lies inside .strtab. */ + const std::string bin = craft_stname_wild(); + const GuardedView gv(bin, /*guard_before=*/false); + check_lookup_rejects(gv.view); +} + +TEST_CASE("is_dynamic_elf PT_INTERP offset+size must not wrap (cand4)", "[elfmal]") +{ + /* machine_elf.cpp is_dynamic_elf(): the check + p_offset + p_filesz > binary.size() is an add-then-check on 64-bit + attacker values; p_offset = -16, p_filesz = 32 wraps to 16 and passes, + and the interpreter string is then read from 16 bytes BEFORE the + buffer. Safe behavior: reject the malformed segment. */ + const std::string bin = craft_interp_wrap(); + const GuardedView gv(bin, /*guard_before=*/true); + REQUIRE_THROWS_AS(tinykvm::is_dynamic_elf(gv.view), tinykvm::MachineException); +} diff --git a/tests/unit/remote.cpp b/tests/unit/remote.cpp index fbd20fc3..40f9d5c5 100644 --- a/tests/unit/remote.cpp +++ b/tests/unit/remote.cpp @@ -467,3 +467,103 @@ int main() { REQUIRE(is_waiting); } } + +TEST_CASE("Remote call with guest-zeroed FSBASE must not wedge fork", "[Remote]") +{ + // Regression test: the VMM tracks an active remote connection using + // guest-writable TLS state (FSBASE-relative). A guest that zeroes its + // own FSBASE before calling into the remote VM makes + // is_remote_connected() read false while the call is still active, + // so the disconnect path throws "Remote VM disconnect called, but + // not connected", leaving the vCPU cross-wired to the remote Machine. + // reset_to() cannot clean that up: the next ordinary remote call on + // the fork fails with "page_at: pt entry not user writable" and the + // fork is permanently wedged. + // The healthy behavior asserted below is: a remote call entered with + // FSBASE=0 completes and disconnects cleanly, and after reset_to() + // a second remote call succeeds (connection count reaching 2). + const auto storage_binary = build_and_load(R"M( +extern long write(int, const void*, unsigned long); +int main() { + return 1234; +} +extern void remote_hello_world() { + write(1, "Hello Remote World!", 19); +} +)M", "-Wl,-Ttext-segment=0x40400000"); + + // Extract storage remote symbols + const std::string command = "objcopy -w --extract-symbol --strip-symbol=!remote* --strip-symbol=* " + storage_binary.first + " storage.syms"; + FILE* f = popen(command.c_str(), "r"); + if (f == nullptr) { + throw std::runtime_error("Unable to extract remote symbols"); + } + pclose(f); + + const auto main_binary = build_and_load(R"M( +extern void remote_hello_world(); +int main() { + return 2345; +} +extern int test_remote_fszero() { + __asm__ volatile("wrfsbase %0" :: "r"(0UL) : "memory"); + remote_hello_world(); + return 100; +} +extern int test_remote_normal() { + remote_hello_world(); + return 100; +} +)M", "-Wl,--just-symbols=storage.syms"); + + tinykvm::Machine storage { storage_binary.second, { + .max_mem = 16ULL << 20, // MB + .vmem_base_address = 1ULL << 30, // 1GB + } }; + storage.setup_linux({"storage"}, env); + storage.run(4.0f); + REQUIRE(storage.return_value() == 1234); + + tinykvm::Machine machine { main_binary.second, { + .max_mem = MAX_MEMORY + } }; + machine.setup_linux({"main"}, env); + machine.remote_connect(storage); + machine.set_remote_allow_page_faults(true); + REQUIRE(machine.has_remote()); + + machine.run(4.0f); + REQUIRE(machine.return_value() == 2345); + + // Create a fork + machine.prepare_copy_on_write(MAX_COWMEM); + tinykvm::Machine fork(machine, { + .max_mem = MAX_MEMORY, + .max_cow_mem = MAX_COWMEM, + .split_hugepages = true + }); + fork.set_remote_allow_page_faults(true); + REQUIRE(fork.has_remote()); + + // A remote call entered with guest FSBASE=0 must complete and + // disconnect cleanly, like any other remote call. + REQUIRE_NOTHROW(fork.vmcall("test_remote_fszero")); + REQUIRE(fork.return_value() == 100); + REQUIRE(!fork.is_remote_connected()); + REQUIRE(fork.remote_connection_count() == 1); + + // reset_to() must restore the fork to a pristine state, so a + // subsequent ordinary remote call succeeds and the connection + // count reaches 2. + fork.reset_to(machine, { + .max_mem = MAX_MEMORY, + .max_cow_mem = MAX_COWMEM, + .split_hugepages = true + }); + REQUIRE(fork.has_remote()); + + REQUIRE_NOTHROW(fork.vmcall("test_remote_normal")); + REQUIRE(fork.return_value() == 100); + REQUIRE(!fork.is_remote_connected()); + REQUIRE(fork.remote_connection_count() == 2); +} diff --git a/tests/unit/reset.cpp b/tests/unit/reset.cpp index 4ee6b23b..97aa48db 100644 --- a/tests/unit/reset.cpp +++ b/tests/unit/reset.cpp @@ -169,3 +169,237 @@ extern void crash(const char *arg) { }); } } + +/* --------------------------------------------------------------------- */ +/* Regression tests for cross-request state leaks through reset_to(). */ +/* A fork is a "request"; reset_to(master) must return it to the master's */ +/* pristine state. Each test below asserts the correct behavior and */ +/* currently FAILS on the unpatched tree: the mutated state survives the */ +/* reset and leaks into the next request. */ +/* --------------------------------------------------------------------- */ +#include +#include +#include +#include +#include +#include + +TEST_CASE("brk is restored to the master value by reset_to", "[Reset]") +{ + /* The guest raises its brk inside a fork (one request). reset_to() must + restore brk to the master's frozen value; currently the raised brk + survives (ratcheted against brk_end), leaking heap growth -- and any + data left on it -- into the next request. */ + const auto binary = build_and_load(R"M( +#define _GNU_SOURCE +#include +#include +int main() { return 0; } +extern long grow_brk(void) { + long cur = syscall(SYS_brk, 0); + syscall(SYS_brk, cur + 0x200000); /* grow by 2MB */ + return syscall(SYS_brk, 0); +} +extern long get_brk(void) { + return syscall(SYS_brk, 0); +})M"); + + tinykvm::Machine machine { binary, { .max_mem = MAX_MEMORY } }; + machine.setup_linux({"reset"}, env); + machine.run(4.0f); + machine.prepare_copy_on_write(0); + + auto fork = tinykvm::Machine { machine, { + .max_mem = MAX_MEMORY, .max_cow_mem = MAX_COWMEM + } }; + + const uint64_t master_brk = machine.brk_address(); + REQUIRE(fork.brk_address() == master_brk); + + /* Request: the guest grows its brk by 2MB. */ + fork.timed_vmcall(fork.address_of("grow_brk"), 2.0f); + REQUIRE(fork.return_value() > master_brk); + + fork.reset_to(machine, { + .max_mem = MAX_MEMORY, + .max_cow_mem = MAX_COWMEM + }); + + /* Next request: brk must be back at the master's value. */ + CHECK(fork.brk_address() == master_brk); + fork.timed_vmcall(fork.address_of("get_brk"), 2.0f); + CHECK(fork.return_value() == master_brk); +} + +TEST_CASE("signal handlers are cleared by reset_to", "[Reset]") +{ + /* A guest that installs a signal handler inside a fork (one request) + must not keep that handler after reset_to(). Currently the handler + leaks: it is still installed after the reset and keeps executing in + the next request. */ + const auto binary = build_and_load(R"M( +#define _GNU_SOURCE +#include +#include +#include +#include +static volatile int counter = 0; +static void handler(int sig) { (void)sig; counter++; } +int main() { return 0; } +extern long install_handler(void) { + struct sigaction sa; memset(&sa, 0, sizeof sa); + sa.sa_handler = handler; + return sigaction(SIGUSR1, &sa, NULL); +} +extern long query_handler(void) { + struct sigaction sa; memset(&sa, 0, sizeof sa); + sigaction(SIGUSR1, NULL, &sa); + return (long)sa.sa_handler; +} +extern long fire_and_count(void) { + counter = 0; + syscall(SYS_tgkill, getpid(), syscall(SYS_gettid), SIGUSR1); + return counter; +})M"); + + tinykvm::Machine machine { binary, { .max_mem = MAX_MEMORY } }; + machine.setup_linux({"reset"}, env); + machine.run(4.0f); + machine.prepare_copy_on_write(0); + + auto fork = tinykvm::Machine { machine, { + .max_mem = MAX_MEMORY, .max_cow_mem = MAX_COWMEM + } }; + + REQUIRE(fork.sigaction(SIGUSR1).is_unset()); + + /* Request: install a SIGUSR1 handler and verify it fires. */ + fork.timed_vmcall(fork.address_of("install_handler"), 2.0f); + REQUIRE(fork.return_value() == 0); + fork.timed_vmcall(fork.address_of("fire_and_count"), 2.0f); + REQUIRE(fork.return_value() == 1); + + fork.reset_to(machine, { + .max_mem = MAX_MEMORY, + .max_cow_mem = MAX_COWMEM + }); + + /* Next request: the handler must be gone (SIG_DFL, never runs). */ + CHECK(fork.sigaction(SIGUSR1).is_unset()); + fork.timed_vmcall(fork.address_of("query_handler"), 2.0f); + CHECK(fork.return_value() == 0); + fork.timed_vmcall(fork.address_of("fire_and_count"), 2.0f); + CHECK(fork.return_value() == 0); +} + +namespace { + /* Two scratch directories, each holding a "rel" file with distinct + content; the host process's cwd is restored on destruction. */ + struct CwdDirs { + std::string dirA, dirB, saved_cwd; + + CwdDirs() + { + char tmplA[] = "/tmp/tinykvm-reset-cwdA-XXXXXX"; + char tmplB[] = "/tmp/tinykvm-reset-cwdB-XXXXXX"; + REQUIRE(mkdtemp(tmplA) != nullptr); + REQUIRE(mkdtemp(tmplB) != nullptr); + dirA = tmplA; + dirB = tmplB; + write_rel(dirA, "AAAA"); + write_rel(dirB, "BBBB"); + char cwdbuf[4096]; + REQUIRE(getcwd(cwdbuf, sizeof(cwdbuf)) != nullptr); + saved_cwd = cwdbuf; + } + ~CwdDirs() + { + chdir(saved_cwd.c_str()); + unlink((dirA + "/rel").c_str()); + unlink((dirB + "/rel").c_str()); + rmdir(dirA.c_str()); + rmdir(dirB.c_str()); + } + static void write_rel(const std::string& dir, const char* data) + { + const int fd = open((dir + "/rel").c_str(), O_WRONLY | O_CREAT, 0644); + REQUIRE(fd >= 0); + REQUIRE(write(fd, data, 4) == 4); + close(fd); + } + }; +} + +TEST_CASE("reset_to preserves the master working directory", "[Reset]") +{ + /* FileDescriptors::reset_to() rebuilds the fd table but never restores + m_current_working_directory{,_fd} from the master, so AT_FDCWD in a + fork resolves against the VMM process's *host* cwd instead of the + master's configured directory. The guest below opens the relative + path "rel" and prints its content plus getcwd(); the correct answer + is the master's directory and file in every case. */ + const auto binary = build_and_load(R"M( +#define _GNU_SOURCE +#include +#include +#include +static char buf[256]; +int main() { return 0; } +extern void do_check(void) { + int fd = open("rel", O_RDONLY); + if (fd < 0) { + write(1, "OPENFAIL", 8); + } else { + int n = read(fd, buf, 4); + close(fd); + if (n > 0) write(1, buf, n); + } + write(1, " cwd=[", 6); + char* r = getcwd(buf, sizeof(buf)); + if (r) write(1, buf, strlen(buf)); + write(1, "]", 1); +})M"); + + CwdDirs dirs; + const std::string expected = "AAAA cwd=[" + dirs.dirA + "]"; + auto approve_all = [](std::string&) { return true; }; + + tinykvm::Machine machine { binary, { .max_mem = MAX_MEMORY } }; + machine.fds().set_open_readable_callback(approve_all); + std::string mout; + machine.set_printer([&](const char* d, size_t s) { mout.append(d, s); }); + machine.setup_linux({"reset"}, env); + machine.run(4.0f); + machine.fds().set_current_working_directory(dirs.dirA); + + /* Sanity: the master itself resolves "rel" against its configured cwd. */ + machine.timed_vmcall(machine.address_of("do_check"), 4.0f); + REQUIRE(mout == expected); + + machine.prepare_copy_on_write(65536); + /* The VMM process's host cwd is now somewhere else, with a different + "rel" file -- resolving against it is the bug. */ + REQUIRE(chdir(dirs.dirB.c_str()) == 0); + + auto fork = tinykvm::Machine { machine, { + .max_mem = MAX_MEMORY, .max_cow_mem = MAX_COWMEM + } }; + /* Re-install the policy callbacks on the fork, as an embedder would. */ + fork.fds().set_open_readable_callback(approve_all); + std::string fout; + fork.set_printer([&](const char* d, size_t s) { fout.append(d, s); }); + + /* A fresh fork must resolve "rel" against the master's cwd. */ + fork.timed_vmcall(fork.address_of("do_check"), 4.0f); + CHECK(fout == expected); + + /* ... and so must the same fork after reset_to(). */ + fork.reset_to(machine, { + .max_mem = MAX_MEMORY, + .max_cow_mem = MAX_COWMEM + }); + fork.fds().set_open_readable_callback(approve_all); + fout.clear(); + fork.timed_vmcall(fork.address_of("do_check"), 4.0f); + CHECK(fout == expected); +} diff --git a/tests/unit/syscalls.cpp b/tests/unit/syscalls.cpp index ca9de020..3094517e 100644 --- a/tests/unit/syscalls.cpp +++ b/tests/unit/syscalls.cpp @@ -529,3 +529,142 @@ int main() { REQUIRE(machine.return_value() == 0); REQUIRE(elapsed < 5.0); } + +TEST_CASE("dup2 onto a fresh virtual fd must succeed and not go stale", "[Syscalls]") +{ + /* Two bugs in one handler. dup2(vfd, new_vfd) translated new_vfd *before* + dup'ing, so an unmanaged target -- the normal case, the guest picked a + free descriptor -- threw and the call returned -EBADF, leaking the + would-be host fd. And when new_vfd *was* managed, the handler registered + new_vfd onto the host fd it had just closed instead of the fresh dup() + result, so the virtual fd aliased a closed (and later recycled) host fd. */ + const auto binary = build_and_load(R"M( +#define _GNU_SOURCE +#include +#include +#include + +int main() { + int fd = open("/scratch", O_RDONLY); + if (fd < 0) return 1; + + /* dup2 onto an unused descriptor must succeed and return it. */ + int rc = dup2(fd, 10); + if (rc != 10) return 2; + + /* The new descriptor must refer to the same open file description. */ + char c = 0; + if (read(10, &c, 1) != 1) return 3; + if (c != 'A') return 4; + + /* dup2 onto an EXISTING descriptor: it must become a fresh reference to + fd's file, not an alias of the host fd that was just closed. */ + int fd2 = open("/scratch", O_RDONLY); + if (fd2 < 0) return 5; + if (dup2(fd, fd2) != fd2) return 6; + if (read(fd2, &c, 1) != 1) return 7; + if (c != 'A') return 8; + + return 0; +})M"); + + ScratchFile file; + tinykvm::Machine machine { binary, { .max_mem = MAX_MEMORY } }; + allow_scratch_file(machine, file); + machine.setup_linux({"dup2-stale-fd"}, env); + machine.run(4.0f); + + REQUIRE(machine.return_value() == 0); +} + + +TEST_CASE("poll with more than 256 valid fds does not throw a foreign exception", "[Syscalls]") +{ + /* The handler accumulated translated fds into a fixed + std::array via .at(). With more than 256 *valid* guest + entries, .at() threw std::out_of_range, which escaped the handler and + Machine::run() itself -- past embedders that (per the documented + contract) only catch MachineException. */ + const auto binary = build_and_load(R"M( +#define _GNU_SOURCE +#include +#include + +#define COUNT 300 + +static struct pollfd pfds[COUNT]; + +int main() { + int pfd[2]; + if (pipe(pfd) < 0) return 1; + /* Make the read end ready, so a successful poll returns immediately. */ + if (write(pfd[1], "x", 1) != 1) return 2; + + for (int i = 0; i < COUNT; i++) { + pfds[i].fd = pfd[0]; + pfds[i].events = POLLIN; + } + long r = poll(pfds, COUNT, 0); + if (r < 0) return 3; + return 0; +})M"); + + tinykvm::Machine machine { binary, { .max_mem = MAX_MEMORY } }; + machine.setup_linux({"poll-overflow"}, env); + + bool threw_machine_exception = false; + try { + machine.run(4.0f); + } catch (const tinykvm::MachineException&) { + /* A clean, documented emulation error is an acceptable outcome. */ + threw_machine_exception = true; + } catch (const std::exception& e) { + FAIL("non-MachineException escaped machine.run(): " << e.what()); + } + + if (!threw_machine_exception) { + REQUIRE(machine.return_value() == 0); + } +} + + +TEST_CASE("futex with an unimplemented op does not throw a foreign exception", "[Syscalls]") +{ + /* The futex handler supports WAIT/WAKE (and the BITSET variants) and threw + std::runtime_error("Unimplemented futex op: ...") for anything else. + That escaped the handler and Machine::run() -- bypassing embedders that + only catch MachineException, and giving the guest no error code at all. + A clean -ENOSYS (or a MachineException) is the acceptable behaviour. */ + const auto binary = build_and_load(R"M( +#define _GNU_SOURCE +#include +#include +#include +#include + +static int futex_word = 0; + +int main() { + /* FUTEX_FD is not implemented by the emulation layer. */ + long r = syscall(SYS_futex, &futex_word, FUTEX_FD, 0, (void*)0, (void*)0, 0); + /* A clean error return (e.g. -ENOSYS) is fine. */ + return (r < 0) ? 0 : 1; +})M"); + + tinykvm::Machine machine { binary, { .max_mem = MAX_MEMORY } }; + machine.setup_linux({"futex-unimplemented-op"}, env); + + bool threw_machine_exception = false; + try { + machine.run(4.0f); + } catch (const tinykvm::MachineException&) { + /* A clean, documented emulation error is an acceptable outcome. */ + threw_machine_exception = true; + } catch (const std::exception& e) { + FAIL("non-MachineException escaped machine.run(): " << e.what()); + } + + if (!threw_machine_exception) { + REQUIRE(machine.return_value() == 0); + } +}