Conversation
jserv
commented
Sep 7, 2026
Contributor
Author
There was a problem hiding this comment.
Benchmarks
Details
| Benchmark suite | Current: 0ef63a0 | Previous: 3ea8cae | Ratio |
|---|---|---|---|
Dhrystone |
2006.75 DMIPS |
1513.333 DMIPS |
0.75 |
CoreMark |
1365.528 iterations/sec |
1067.054 iterations/sec |
0.78 |
This comment was automatically generated by workflow using github-action-benchmark.
mpool_create() accepts caller-supplied pool and chunk sizes, and two of its guards did not hold for the edges of that input range. The guard meant to ensure a pool holds at least one chunk added the header size to pool_size instead of raising it to a full slot, so a pool smaller than one chunk still computed a chunk count of zero. The free-list builder then ran a loop bounded by "chunk_count - 1" on a size_t, wrapped to SIZE_MAX, and wrote pointers off the end of the arena. No in-tree caller reaches this, but the function is public and the guard exists precisely to prevent it. Payload alignment was also left to chance. Chunks are handed out one header into each slot, so the slot stride decides their alignment; a chunk size that is not a multiple of the header size produced payload pointers that drifted out of alignment as the arena was walked. The three in-tree pools pass sizeof() of 8-aligned structs and were never affected, but nothing enforced or documented that. Fold the duplicated free-list construction in mpool_create() and mpool_extend() into one helper while here. The two copies had already drifted: the extend copy omitted the NULL terminator, harmless only because chunk_count happens to guard the pop, and exactly the kind of divergence a second copy invites. Pools now also size themselves from the page-rounded arena rather than the requested size, recovering up to a page per pool that was mapped and never handed out.
An ELF file is untrusted input, but is_valid() checked only the magic, the class, and the machine type. Nothing else was verified, and raw_size was recorded and then never used to bound anything, so every header traversal indexed straight off the mapping with file-supplied offsets. A malformed file produced out-of-bounds reads throughout, including strcmp walking off the end of the buffer looking for a terminator, and a file shorter than an ELF header was already out of bounds inside the magic comparison. Validate the whole structure once at the point where the file is accepted, so no traversal downstream has to re-check. Both header tables and every section and segment they claim to hold must lie in the file, string-table sections must be non-empty and end in a NUL, and the tables must be 4-aligned because their entries are cast to structs holding uint32_t. Requiring alignment is not pedantry: e_phoff is attacker controlled, and a misaligned table is undefined behavior that a sanitizer build traps on. A PT_LOAD segment must also declare p_filesz no larger than p_memsz. elf_load() derives its zero-fill length from max(p_memsz, p_filesz), so an inverted pair clears guest memory past the end of the segment and over whatever was loaded there. Sections are confirmed by type at each point of use as well as during validation. Looking a section up by name while validating it by type is how a section named .symtab but typed SHT_PROGBITS would otherwise reach a cast to struct Elf32_Sym without ever passing the alignment check.
Both MMIO accessors end in a default case that treats any unmatched register as an index into the device configuration struct, computed as the register number minus VIRTIO_Config with no range check. The offset arrives masked to 20 bits, so it spans a 1 MiB window while the struct is a few dozen bytes on the heap. Anything below the config window wraps the subtraction to a huge index and anything above it runs off the allocation, in both directions, from a plain guest store. The read path hands host heap back to the guest the same way. An ASan harness driving two such offsets faults inside virtio_blk_write. Range check the index at both ends and fail the device when a guest names a register outside its configuration space.
The descriptor handler read the payload address, length, and sector out of the guest's virtqueue and passed them to memcpy after checking only the sector. A guest could name a valid sector with an arbitrary length and run off the end of the disk mapping on write, or off the end of guest RAM on read, and a payload address was added to the RAM base with no range check at all. The emulator segfaults on the first such request. The sector check could also be defeated outright. Capacity is left at zero when no disk image is attached, and the test read "sector > capacity - 1", which wraps to UINT64_MAX and admits every sector onto a null disk pointer. Validate every range derived from guest input against RAM, in the same shape virtio-rng already used, and bound the payload against the disk from the sector offset. Require each descriptor to be long enough for what is read or written through it, and reject an index that does not name a slot in the table, which virtio-rng also does. The status descriptor is validated before the status byte is written, since reporting an error through an unchecked pointer is not a way to report an error. Bound the request against host-owned geometry rather than the configuration space, which the guest can write: a driver that sets capacity to UINT64_MAX would otherwise pass every sector check. The config struct stays as the guest-visible mirror and is now written but never read back for any decision. Sizing an mmap, a malloc, or the teardown munmap from the mirror was the same mistake, so those take the host copy too. Two adjacent defects in setup: disk_size was left uninitialized and, on Emscripten, every assignment to it in the /dev branch is preprocessed away, so its value sized an mmap. A zero-length image also wrapped the capacity round-up. Both are now rejected with a diagnostic. The descriptor is copied rather than cast, because the guest chooses the table base and the entry is only guaranteed 4-byte aligned while the struct wants 8.
A declaration written with an empty parameter list says nothing about the arguments a function takes, which is why the compiler does not treat it as a prototype and warns about the definition having none. Seventeen declarations and their definitions were written this way. Give them (void). Beyond silencing the warning, C23 changes what an empty list means, so spelling the intent out now avoids a silent shift in meaning later. Two functions in jit.c had external linkage with no declaration in any header. One is used only from a file included into the same translation unit and the other only from its own; both are now static. reset_rv_run_state() is declared in em_runtime.h behind an Emscripten guard and called only from the Emscripten teardown path, so its definition now sits behind the same guard rather than being built into every target with no prototype in scope.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An audit of the first-party sources turned up three places where untrusted input reaches memory operations without being bounded. This fixes them, and adds a prototype sweep that fell out of the same warning pass.
The serious one is virtio-blk. The descriptor handler took the payload address and length straight from the guest's virtqueue and passed them to
memcpy, checking only the sector. A guest can name a valid sector with an arbitrary length and run off the end of the disk mapping on write, or off the end of guest RAM on read. The sector check itself could be bypassed: capacity is deliberately left at zero when no disk image is attached, and the test was written assector > capacity - 1, which wraps toUINT64_MAXand admits every sector onto a null disk pointer. Ranges are now validated in the same shape virtio-rng already used, the payload is bounded against the disk from the sector offset, and the status descriptor is checked before the status byte is written, since reporting an error through an unchecked pointer is not a way to report an error.The ELF loader validated the magic, the class, and the machine type, and nothing else.
raw_sizewas recorded and never used, so every header traversal indexed off the mapping with file-supplied offsets, includingstrcmpwalking past the end of the buffer looking for a terminator. Validation now happens once where the file is accepted: both header tables and everything they claim to hold must lie in the file, string tables must end in a NUL, and the tables must be 4-aligned because their entries are cast to structs holdinguint32_t. Sections are confirmed by type at each use as well, since looking a section up by name while validating it by type is how a section named.symtabbut typedSHT_PROGBITSreaches a cast without passing the alignment check.In mpool, the guard meant to ensure a pool holds at least one chunk added the header size rather than raising the pool to a full slot, so an undersized pool computed a chunk count of zero and the free-list loop bound wrapped to
SIZE_MAX. No in-tree caller reaches it, but the function is public. Payload alignment is now enforced rather than assumed, and the duplicated free-list construction is folded into one helper; the two copies had already drifted.Verified by building defconfig, mini, jit, system, and wasm, and by
make checkboth plain and withENABLE_UBSAN=1. Linux boots with a virtio-blk image attached and completes 512-byte and 4 KiB reads and writes. Injecting a hostile payload length at the point a malicious driver would place it segfaults the emulator before this series and halts the device while leaving the emulator alive after it. Fuzzing the ELF loader over 3000 structural mutations under ASan and UBSan went from 1396 crashes and no rejections to no crashes and 2702 rejections, with all ten prebuilt ELFs still loading; five hand-crafted type-confusion cases behave as intended. mpool has unit, randomized-churn, and TSan coverage. Each of the four commits builds on its own under both defconfig and system_defconfig.Left out deliberately: 30 remaining
-Wcast-alignsites. They arecontainer_ofandoffsetofpatterns over malloc'd or page-aligned memory where the invariant holds but the compiler cannot see it, and the warning is not in the project's flag set. The two that were reachable with attacker-controlled misalignment, the ELF tables and the virtio descriptor read, are fixed here.Summary by cubic
Fixes memory-safety defects where untrusted input reached memory operations without bounds checks: a malicious virtio-blk guest could crash the emulator, and a malformed ELF could drive out-of-bounds reads throughout the loader. Also fixes a sizing bug in
mpoolthat could walk off the pool arena, and tightens empty parameter lists to(void).Bug Fixes
p_fileszvsp_memszonce at load; symbol-table lookups confirm section type.mpoolraises undersized pools to at least one slot, aligns payloads, and builds freelists from the page-rounded arena.Refactors
(void); two file-local JIT helpers arestatic, andreset_rv_run_state()sits behind the Emscripten guard it is declared under.Written for commit 0ef63a0. Summary will update on new commits.