Repository navigation
fix(tracer): make the stack canary check match the linker layout - #1874
Merged
Merged
Conversation
The linker script places the 128 byte canary at [__stack_bottom, __stack_bottom + 128) with __stack_bottom == stack_end, and the stack proper starts right after it. assert_effective_address flagged (stack_end, stack_end + 128] instead, so a store to the lowest stack doubleword panicked as a stack overflow while a store to the first canary byte went through. Use the half-open range and add two regression tests.
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.
Summary
The tracer's stack canary check is off by one relative to the memory layout the linker script and the book describe.
src/linker.ld.templatesets__stack_bottom = ALIGN(8)after__program_end, puts the 128 byte canary at[__stack_bottom, __stack_bottom + 128), and starts the stack right after it. On the host,MemoryLayout::newsetsstack_end = RAM_START_ADDRESS + program_sizewithprogram_size = program_end - RAM_START_ADDRESS. So whenever the image ends 8 byte aligned, which is the normal case,stack_end == __stack_bottomand the canary is the half-open range[stack_end, stack_end + 128).Mmu::assert_effective_addresschecked(stack_end, stack_end + 128]instead:So a store to the lowest doubleword of the stack (
stack_end + 128) panics with "Stack overflow ... Increase stack_size", while a store to the first canary byte (stack_end) is not caught.Reproduced end to end: a minimal RV64 guest linked with the repo's own
linker.ld.template(__stack_bottom = __program_end = 0x80000048) that stores to__stack_bottom + 128panics in the tracer withStack overflow: attempted to write to 0x800000C8, which is in the stack canary region. With the fix it traces normally. In practice a guest whose deepest frame uses exactly all of its configured stack cannot be traced.When the image does not end 8 byte aligned, the linker adds up to 7 bytes of padding before
__stack_bottom. The new check then covers that padding plus the first 121 to 127 canary bytes, and still never rejects a store inside the stack proper, so it is never stricter than the linker layout. Making it exact in that case would need the host to roundprogram_sizeup the same way the linker does; I left that out to keep this change minimal, but I am happy to add it if you prefer.Changes
tracer/src/emulator/mmu.rs: useea < layout.stack_end || ea >= layout.stack_end + STACK_CANARY_SIZE, with a comment stating the range and where it comes from.test_mmu:test_stack_overflow_first_canary_byte(a store tostack_endmust panic) andtest_lowest_stack_word_is_writable(a doubleword store and load atstack_end + STACK_CANARY_SIZEmust succeed).Testing
Ran tests for modified crates
cargo clippyandcargo fmtpassBoth new tests fail on
mainand pass with the fix; all 7test_mmutests pass, including the existingstack_end + 1case.Mutation checks: reverting only the lower bound to
<=failstest_stack_overflow_first_canary_byte, reverting only the upper bound to>failstest_lowest_stack_word_is_writable.cargo test -p tracer: 127 passed with the fix vs 125 onmain. The same 12 tests fail in both runs, all inbuild_muldiv_guest, because they need thejoltCLI and a RISC-V guest toolchain that my environment does not have.cargo clippy -p tracer --all-targets -- -D warningsandcargo fmt --checkare clean.Security Considerations
No effect on proof soundness or verifier correctness. The check lives only in the reference tracer's MMU and decides whether tracing a guest panics; it does not change what is committed or proven. The change makes the tracer accept one more legal store (the lowest stack word) and reject one it previously let through (the first canary byte).
Breaking Changes
None