This repository has been archived by the owner on Mar 24, 2022. It is now read-only.
Fix incorrect globals/sigstack allocation layout #401
Merged
Conversation
This file contains 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
iximeow
previously approved these changes
Jan 24, 2020
Most of this patch is actually to fix up the tests that were erroneously succeeding because of this bug. The core of the fix is here in `mmap.rs`: ``` - let sigstack = globals + host_page_size(); + let sigstack = globals + region.limits.globals_size + host_page_size(); ``` This was causing the signal stack to begin one page after the start of the globals section, regardless of how big the globals are. This mostly wasn't causing problems because the default globals size is a page, and we _were_ still adding in a page for the guard, meaning the segments didn't actually overlap with default `Limits`. However, the guard page for the signal stack was missing, and configurations with larger globals sizes could end up overlapping the signal stack, potentially allowing guests to observe residual signal stack values. The lack of a signal stack guard page was _also_ making it so that tests running in debug mode would succeed, even though the signal handler was overflowing its stack into the globals page. To make those tests pass with the correct memory layout, the signal stack size is now configurable and set higher by default on debug builds (`cfg(debug_assertions)`). On release mode, the stack use is fine as the problematic series of stack allocations get optimized into some straightforward copies, so we continue to use SIGSTKSZ as a default in that configuration. Finally, I discovered some linking errors with the tests on release mode due to overly-aggressive stripping, so I had to refactor a couple tests.
Also adds a test to ensure that a fault in the sigstack guard page is fatal
acfoltzer
force-pushed
the
acf/alloc-layout-fix
branch
from
96cd46b
to
1d2dd6d
Compare
acfoltzer
force-pushed
the
acf/alloc-layout-fix
branch
from
1d2dd6d
to
7064e22
Compare
iximeow
approved these changes
Jan 24, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Most of this patch is actually to fix up the tests that were erroneously succeeding because of this bug. The core of the fix is here in
mmap.rs:This was causing the signal stack to begin one page after the start of the globals section, regardless of how big the globals are. This mostly wasn't causing problems because the default globals size is a page, and we were still adding in a page for the guard, meaning the segments didn't actually overlap with default
Limits. However, the guard page for the signal stack was missing, and configurations with larger globals sizes could end up overlapping the signal stack, potentially allowing guests to observe residual signal stack values.The lack of a signal stack guard page was also making it so that tests running in debug mode would succeed, even though the signal handler was overflowing its stack into the globals page. To make those tests pass with the correct memory layout, the signal stack size is now configurable and set higher by default on debug builds (
cfg(debug_assertions)). On release mode, the stack use is fine as the problematic series of stack allocations get optimized into some straightforward copies, so we continue to use SIGSTKSZ as a default in that configuration.Finally, I added a test that tries to write to the sigstack guard page, improved the "is this a fatal fault address?" method, and made sure that all tests run correctly in release mode.