cranelift: Include clobbers and outgoing args in stack limit #8301
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
github-actions
bot
added
cranelift
When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees. The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in `wasmparser::limits`, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB. This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured `max_wasm_stack` size, and getting within 16kB of the host stack size is too small to be safe even with this fixed. However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case. (For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for `max_wasm_stack`, so that is too small with or without this bug fix.)
jameysharp
force-pushed
the
fix-stack-limit
branch
from
806080f to
52ebb71
Compare
|
This makes sense to me, yes. |
alexcrichton
pushed a commit
to alexcrichton/wasmtime
that referenced
this pull request
Apr 11, 2024
…ealliance#8301) When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees. The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in `wasmparser::limits`, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB. This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured `max_wasm_stack` size, and getting within 16kB of the host stack size is too small to be safe even with this fixed. However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case. (For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for `max_wasm_stack`, so that is too small with or without this bug fix.)
alexcrichton
pushed a commit
to alexcrichton/wasmtime
that referenced
this pull request
Apr 11, 2024
…ealliance#8301) When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees. The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in `wasmparser::limits`, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB. This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured `max_wasm_stack` size, and getting within 16kB of the host stack size is too small to be safe even with this fixed. However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case. (For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for `max_wasm_stack`, so that is too small with or without this bug fix.)
alexcrichton
pushed a commit
to alexcrichton/wasmtime
that referenced
this pull request
Apr 11, 2024
…ealliance#8301) When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees. The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in `wasmparser::limits`, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB. This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured `max_wasm_stack` size, and getting within 16kB of the host stack size is too small to be safe even with this fixed. However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case. (For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for `max_wasm_stack`, so that is too small with or without this bug fix.)
alexcrichton
pushed a commit
to alexcrichton/wasmtime
that referenced
this pull request
Apr 11, 2024
…ealliance#8301) When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees. The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in `wasmparser::limits`, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB. This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured `max_wasm_stack` size, and getting within 16kB of the host stack size is too small to be safe even with this fixed. However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case. (For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for `max_wasm_stack`, so that is too small with or without this bug fix.)
alexcrichton
added a commit
that referenced
this pull request
Apr 11, 2024
* cranelift: Include clobbers and outgoing args in stack limit (#8301) When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees. The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in `wasmparser::limits`, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB. This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured `max_wasm_stack` size, and getting within 16kB of the host stack size is too small to be safe even with this fixed. However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case. (For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for `max_wasm_stack`, so that is too small with or without this bug fix.) * fix: bindgen trappable_errors using unversion/versioned packages (#8305) Signed-off-by: Brian H <brian.hardock@fermyon.com> * Cranelift: Do not dedupe/GVN bitcasts from reference values (#8317) * Cranelift: Do not dedupe/GVN bitcasts from reference values Deduping bitcasts to integers from references can make the references no long longer live across safepoints, and instead only the bitcasted integer results would be. Because the reference is no longer live after the safepoint, the safepoint's stack map would not have an entry for the reference, which could result in the collector reclaiming an object too early, which is basically a use-after-free bug. Luckily, we sandbox the GC heap now, so such UAF bugs aren't memory unsafe, but they could potentially result in denial of service attacks. Either way, we don't want those bugs! On the other hand, it is technically fine to dedupe bitcasts *to* reference types. Doing so extends, rather than shortens, the live range of the GC reference. This potentially adds it to more stack maps than it otherwise would have been in, which means it might unnecessarily survive a GC it otherwise wouldn't have. But that is fine. Shrinking live ranges of GC references, and removing them from stack maps they otherwise should have been in, is the problematic transformation. * Add additional logging and debug asserts for GC stuff * Handle out-of-bounds component sections (#8323) * Handle out-of-bounds component sections Fixes #8322 * Add a test that trancated component binaries don't cause panics --------- Signed-off-by: Brian H <brian.hardock@fermyon.com> Co-authored-by: Jamey Sharp <jsharp@fastly.com> Co-authored-by: Brian <brian.hardock@fermyon.com> Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
alexcrichton
added a commit
that referenced
this pull request
Apr 11, 2024
…8335) When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees. The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in `wasmparser::limits`, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB. This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured `max_wasm_stack` size, and getting within 16kB of the host stack size is too small to be safe even with this fixed. However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case. (For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for `max_wasm_stack`, so that is too small with or without this bug fix.) Co-authored-by: Jamey Sharp <jsharp@fastly.com>
alexcrichton
added a commit
that referenced
this pull request
Apr 11, 2024
…8336) When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees. The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in `wasmparser::limits`, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB. This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured `max_wasm_stack` size, and getting within 16kB of the host stack size is too small to be safe even with this fixed. However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case. (For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for `max_wasm_stack`, so that is too small with or without this bug fix.) Co-authored-by: Jamey Sharp <jsharp@fastly.com>
alexcrichton
added a commit
that referenced
this pull request
Apr 11, 2024
…8334) When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees. The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by `MAX_WASM_FUNCTION_PARAMS` in `wasmparser::limits`, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB. This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured `max_wasm_stack` size, and getting within 16kB of the host stack size is too small to be safe even with this fixed. However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case. (For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for `max_wasm_stack`, so that is too small with or without this bug fix.) Co-authored-by: Jamey Sharp <jsharp@fastly.com>
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.
When we compute the amount of space that we need in a stack frame for the stack limit check, we were only counting spill-slots and explicit stack-slots. However, we need to account for all uses of the stack which occur before the next stack limit check. That includes clobbers and any stack arguments we want to pass to callees.
The maximum amount that we could have missed by is essentially bounded by the number of arguments which could be passed to a function. In Wasmtime, that is limited by
MAX_WASM_FUNCTION_PARAMSinwasmparser::limits, which is set to 1,000, and the largest arguments are 16-byte vectors, so this could undercount by about 16kB.This is not a security issue according to Wasmtime's security policy (https://docs.wasmtime.dev/security-what-is-considered-a-security-vulnerability.html) because it's the embedder's responsibility to ensure that the stack where Wasmtime is running has enough extra space on top of the configured
max_wasm_stacksize, and getting within 16kB of the host stack size is too small to be safe even with this fixed.However, this was definitely not the intended behavior when stack limit checks or stack probes are enabled, and anyone with non-default configurations or non-Wasmtime uses of Cranelift should evaluate whether this bug impacts your use case.
(For reference: When Wasmtime is used in async mode or on Linux, the default stack size is 1.5MB larger than the default WebAssembly stack limit, so such configurations are typically safe regardless. On the other hand, on macOS the default non-async stack size for threads other than the main thread is the same size as the default for
max_wasm_stack, so that is too small with or without this bug fix.)cc: @uweigand @bjorn3 @afonso360