Stop brk growing through its neighbors - #395
Merged
Merged
Conversation
sys_brk bounded a new break only by guest_size. Everything from the heap up to MMAP_RX_BASE is one premapped RW span and a grow re-maps what it crosses, so an unbounded grow did not fault: it extended page tables over the guest stack, zeroed the range it had just handed the guest, and returned success. The guest saw its own stack replaced by zeroes, surfacing later as corrupted heap metadata or a jump through a null pointer. Reproduced at about 104 MiB of malloc, which is where the heap reaches the stack. A grow now stops at the first tracked region above the break, with g->stack_base as the bound at the stack, and returns the unchanged break, which glibc and musl read as no more heap before falling back to mmap. Nothing is exempt from that, the heap's own record included: two drafts exempted it by name and both were reachable, because mremap keeps the source region's name. regions[] is start-sorted and non-overlapping, so ends are monotonic and guest_region_first_end_above answers in one search. The record it lands on is the first that can be in the way, a straddler included, and the record a growth extends is the one below it. Walking every record below the break instead answers the same question and costs 1.47 ns per record: 6221 ns against 1823 once 3000 heap records sit below it. Four defects around it were found the same way and fixed here. A heap hole left the pieces above it stranded, which wedged the heap for the life of the process in 48 measured shapes. A growth adopted a sealed neighbor's prot, so the next mprotect took its same-prot fast path and left the pages writable. The fallback record spanned pages that had no page-table entries. The stale-tracker flag outlived the execve that rebuilt the table it doubted, which static glibc does not survive, since its pre-TLS __libc_setup_tls writes errno before the thread pointer exists. Image placement leaves BRK_MIN_WINDOW above the break, so an image whose load_max is 2 MiB-aligned no longer gets a heap of zero bytes, and is bounded by MMAP_RX_BASE rather than the infra reserve, so an ET_EXEC linked near 250 MiB is refused instead of placing its stack on the mmap RX region. execve asks for that placement before its point of no return and answers ENOEXEC, where the post-PNR refusal would have taken the VM down with exit(128). A terminating EL0 fault now reports its address, PC and ESR at warn level rather than only under --verbose, for SIGILL as well as SIGSEGV. That is the diagnostics gap the issue also named: the reporter had to rebuild with the line promoted to see it at all. src/proved/brk.h carries the bound arithmetic under ACSL contracts, and make verify-brk discharges 11 of 11 obligations with both mutants caught. tests/test-brk-stack.c covers every neighbor shape in 26 checks, each verified to fail against the defect it names, and runs in the elfuse-aarch64 and qemu-aarch64 lanes so a real kernel adjudicates it. Closes #320
The catch-all branch in vcpu_handle_el0_fault delivers SIGILL for every EC that is not a lower-EL abort, and its message named one of them. That was harmless while the line only appeared under --verbose; now that a terminating fault reports at warn level, a guest killed by an alignment fault or an illegal execution state gets a default-level line claiming an undefined instruction. Say what the branch actually covers and keep the EC, which is what identifies the class. The comment above it needed the same treatment for a different reason: it listed 0x18 and 0x3C among the classes landing here, but src/core/shim.S dispatches those to HVC #12 and HVC #10 before the catch-all, so neither ever arrives.
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.
sys_brkbounded a new break only byguest_size. Everything from the heap up toMMAP_RX_BASEis one premapped RW span and a grow re-maps what it crosses, so an unbounded grow did not fault: it extended page tables over the guest stack, zeroed the range it had just handed the guest, and returned success. The guest saw its own stack replaced by zeroes, which surfaces later and elsewhere as corrupted heap metadata or a jump through a null pointer.A grow now stops at the first tracked region above the break and returns the unchanged break, which glibc and musl read as "no more heap" before falling back to mmap. Nothing is exempt from that, the heap's own record included: two drafts exempted it by name and both turned out to be reachable, because
mremapkeeps the source region's name.Four more defects around it were found the same way and are fixed here. A heap hole left the pieces above it stranded, wedging the heap for the life of the process in 48 measured shapes. A growth adopted a sealed neighbour's prot, so the next
mprotecttook its same-prot fast path and left the pages writable. The fallback record spanned pages that had no page-table entries. The stale-tracker flag outlived theexecvethat rebuilt the table it doubted, which static glibc does not survive. Image placement also leaves a minimum window above the break and is bounded byMMAP_RX_BASE, so an ET_EXEC linked near 250 MiB is refused rather than placing its stack on the mmap RX region, andexecveasks for that placement before its point of no return so the guest gets ENOEXEC instead of the VM going down withexit(128).A terminating EL0 fault now reports its address, PC and ESR at warn level rather than only under
--verbose, for SIGILL as well as SIGSEGV. That is the diagnostics gap #320 also named: the reporter had to rebuild with the line promoted before they could see it.Left out deliberately: a capacity pre-merge in
guest_region_add_ex_owned_gpawas written and then removed, because no guest-reachable path exercised it. Five follow-ups are filed rather than folded in, including a fatal signal that ends only the faulting thread instead of the thread group, and the 104 MiB heap window itself, which this bounds rather than widens.Closes #320
Summary by cubic
Fixes
sys_brkgrowing through its neighbors, which could overwrite the guest stack and silently corrupt heap metadata (issue #320).execve.execverefuses images that leave no room withENOEXECinstead ofexit(128).--verbose; the SIGILL line describes the branch as a non-abort exception and carries the EC rather than claiming an undefined instruction.make check, the full test matrix,make verify-brk(11 of 11 obligations), and the newtest-brk-stacksuite all pass.Written for commit 4f200a9. Summary will update on new commits.