Add bounds check in VMContinuationStack::initialize for args buffer size#13662
Conversation
2b8f887 to
c329b8f
Compare
VMContinuationStack::initialize writes control data and an args buffer at offsets below the top of the stack, but did not verify that the total size fits within the allocated stack. A WebAssembly module with a continuation function type with a very large number of parameters or return values could cause writes past the guard page into adjacent memory mappings. This commit adds: - Checked arithmetic (checked_mul + checked_add) for the args buffer size calculation to prevent overflow. - A bounds check ensuring the total control data size (0x40 bytes of fixed header + args buffer) does not exceed the stack allocation. - Propagation of the new Result return type through the stack.rs wrapper and the cont_new caller. The dummy implementation (for unsupported platforms) is updated to match the new signature. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Adds a test that creates a continuation function type with 4,500 parameters on a 64 KiB continuation stack, verifying that cont.new returns an error instead of writing past the stack allocation. The test skips gracefully on platforms where stack switching is not supported (e.g., macOS aarch64, Pulley). Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
c329b8f to
876e35c
Compare
| let args_data_size = | ||
| usize::try_from(args_capacity).unwrap() * std::mem::size_of::<ValRaw>(); | ||
| let args_data_size = usize::try_from(args_capacity) | ||
| .unwrap() |
There was a problem hiding this comment.
Since this function now returns Result could this use ? instead of .unwrap()?
There was a problem hiding this comment.
Done, replaced all three .unwrap() calls in initialize with ?.
| // Ensure the control data (fixed header + args buffer) fits | ||
| // within the stack allocation. Without this check, a | ||
| // high-arity function type could write past the guard page | ||
| // into adjacent memory. | ||
| let total_control_size = 0x40 + args_data_size; |
There was a problem hiding this comment.
Could this avoid the dance here of adding/subtracting/adding 0x40 by setting the original computation to total_control_size, then calculating args_data_size by subtracing 0x40 from that?
There was a problem hiding this comment.
Good call. Now computes total_control_size directly from the checked arithmetic, then derives args_data_size = total_control_size - 0x40. Also replaced tos.sub(0x40 + args_data_size) with tos.sub(total_control_size) in the to_store array for consistency.
- Compute total_control_size directly instead of the add/subtract/add dance with 0x40 - Replace .unwrap() with ? on usize::try_from calls since initialize now returns Result - Add assert!(!cfg!(target_arch = "x86_64")) to the test skip guard so x86_64 never silently skips the test Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks! Happy to merge once CI is green
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
|
||
| let Ok(engine) = Engine::new(&config) else { | ||
| // Stack switching is not supported on all platforms; skip gracefully. | ||
| assert!(!cfg!(target_arch = "x86_64")); |
There was a problem hiding this comment.
Ah, sorry, this'll need adjusting to:
assert!(!(cfg!(target_arch = "x86_64") && cfg!(unix)));There was a problem hiding this comment.
Done, updated to check both conditions.
Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Fix the bounds check in VMContinuationStack::initialize to compare against usable stack space rather than total allocation size. Prior to bytecodealliance#13662, initialize() had no bounds check at all, so a high-arity function type (e.g. 600 params on an 8192-byte stack) would unconditionally write control data into the guard page and segfault. PR bytecodealliance#13662 added a bounds check, but compared against self.len which for Mmap allocations includes the guard page. This meant the 600-param case still slipped through: 9664 <= 12288 passed the check, but the write still landed in the non-writable guard page. This fix subtracts the guard page size for Mmap allocations so the check reflects the actual usable stack space. Custom allocations are unaffected since their len does not include a guard page. Adds a regression test matching the exact scenario from bytecodealliance#13703 (600 params on an 8192-byte stack). Closes bytecodealliance#13703 Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
…iance#13704) Fix the bounds check in VMContinuationStack::initialize to compare against usable stack space rather than total allocation size. Prior to bytecodealliance#13662, initialize() had no bounds check at all, so a high-arity function type (e.g. 600 params on an 8192-byte stack) would unconditionally write control data into the guard page and segfault. PR bytecodealliance#13662 added a bounds check, but compared against self.len which for Mmap allocations includes the guard page. This meant the 600-param case still slipped through: 9664 <= 12288 passed the check, but the write still landed in the non-writable guard page. This fix subtracts the guard page size for Mmap allocations so the check reflects the actual usable stack space. Custom allocations are unaffected since their len does not include a guard page. Adds a regression test matching the exact scenario from bytecodealliance#13703 (600 params on an 8192-byte stack). Closes bytecodealliance#13703 Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
VMContinuationStack::initializewrites control data and an args buffer at offsets below the top of the continuation stack, but does not verify that the total size fits within the allocated stack region.The problem
A Wasm module can define a continuation function type with a large number of parameters or return values. When
cont.newis executed, the call chain reachesinitialize()(stack/unix.rs:226), which computes:args_data_size = max(param_count, return_count) * size_of::<ValRaw>()The fixed header occupies 0x40 (64) bytes. If the total control data size (header + args buffer) exceeds the stack allocation, the write loop stores values past the stack region into adjacent memory. With the default 2 MiB stack, this requires 131,069+ params; with a smaller
async_stack_size, fewer params suffice (e.g., 800 params on an 8 KiB stack).Without this fix, the out-of-bounds write hits the guard page and crashes with SIGSEGV. With the fix, a clean error is returned instead.
The fix
checked_mul+checked_add, returning an error on arithmetic overflow.ensure!(total_control_size <= self.len)) before any writes, so a high-arity function type produces a clean trap instead of a crash.initializefrom()toResult<()>and propagate the error through thestack.rswrapper and thecont_newcaller (which already returnsResult).Test
A regression test (
stack_switching_cont_new_high_arity_rejected) creates a function type with 800i32params on an 8 KiB continuation stack, verifying thatcont.newreturns an error instead of crashing. The test skips gracefully on platforms where stack switching is not supported.Bug origin
Introduced in #10388 ("Stack switching: Infrastructure and runtime support", Frank Emrich, 2025-06-04).