Skip to content

safely_within() lacks within()'s wraparound check: host OOB read from the safe syscall set on non-CoW machines #98

Description

@perbu

Summary

Memory::safely_within() (lib/tinykvm/memory.hpp:78) is missing the
wraparound guard that its sibling within() (:72) has, so a guest pointer
whose addr + asize wraps 2^64 passes validation and the identity-map fast
path in copy_from_guest() reads outside the guest mapping.

/* Unsafe */
bool within(uint64_t addr, size_t asize) const noexcept {
    return (addr >= physbase) && (addr + asize <= physbase + this->size) && (addr <= addr + asize);
}                                                                          // ^^^^^^^^^^^^^^^^^^^^
/* Safe */
bool safely_within(uint64_t addr, size_t asize) const noexcept {
    return (addr >= safebase) && (addr + asize <= physbase + this->size);
}                                                                          // guard absent

Mechanism

With addr = 0xFFFFFFFFFFFFF000 and a nonzero asize, addr + asize wraps to
a small value, so the second conjunct holds and safely_within() returns true.
copy_from_guest() (machine_utils.cpp:104,121) then takes its identity-map
fast path and memcpys from an address below the mapping.

Forks are not affected: uses_cow_memory() forces the validating page-walk
path instead. The exposure is non-CoW machines — a plain master, which is
what simplekvm, most of the unit tests, and the vmod master during warmup
all are.

Reachability

Any handler in the default safe set that hands a raw guest pointer to
copy_from_guest. Reproduced from two:

  • SYS_sigaltstack(ss = 0xFFFFFFFFFFFFFFF0)system_calls.cpp:841
  • writev(1, {iov_base = 0xFFFFFFFFFFFFF000, iov_len = 8192}, 1)system_calls.cpp:672

Both give an ASan SEGV at machine_utils.cpp:122. The identical writev on a
fork throws a clean MemoryException, which is the correct behaviour.

Impact

Deterministic host SIGSEGV from the safe syscall set. On a host where the
region below the mapping happens to be mapped, a silent out-of-bounds host
read instead.

Fix

One line, mirroring within():

bool safely_within(uint64_t addr, size_t asize) const noexcept {
    return (addr >= safebase) && (addr + asize <= physbase + this->size) && (addr <= addr + asize);
}

Verified in a patched build: the crash becomes a clean MemoryException.

Note on precedent and on testing

memzero() had this same defect and was fixed locally in the caller rather
than in the accessor, which is why it survived here.

No regression test landed for this one. It needs a master-mode (non-CoW)
machine, and it is invisible to ASan anyway — raw-mmap guest memory has no
redzones, so only a functional assertion on the return value catches it. The
right vehicle is a non-CoW mode in fuzz/syscall_fuzz.cpp; we're preparing
that as a separate harness PR.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions