diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 6fd4fe6..2518287 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 0000000..ffe85d6 --- /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 fbd20fc..40f9d5c 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 4ee6b23..97aa48d 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 ca9de02..3094517 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); + } +}