1747870 - 11-bit field of wasm stack map is not large enough on x86

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Lars T Hansen [:lth]]

I was experimenting somewhat innocently with adding assertions for invariants that are only documented in comments. A comment asserts that the 11 bits of the offset field is large enough because MaxParams * MaxParamSize / sizeof(void*) < (1 << 11). We can test this, the max parameter size is 16 for a v128 datum:

diff --git a/js/src/wasm/WasmGC.h b/js/src/wasm/WasmGC.h
--- a/js/src/wasm/WasmGC.h
+++ b/js/src/wasm/WasmGC.h
@@ -107,6 +107,15 @@ struct StackMap final {
   static constexpr uint32_t maxExitStubWords = (1 << 6) - 1;
   static constexpr uint32_t maxFrameOffsetFromTop = (1 << 11) - 1;
 
+  // The value 16 is the size of a SIMD datum, the largest parameter.
+  // FIXME: Surely we can get that value from somewhere?
+  //
+  // Add 16 words to account for the size of FrameWithTls including any shadow
+  // stack (at worst 8 words total), and then a little headroom in case the
+  // argument area had to be aligned.
+  static_assert(maxFrameOffsetFromTop >= (MaxParams * 16 / sizeof(void*)) + 16,
+                "11 bits in the offset field");
+
   uint32_t bitmap[1];
 
   explicit StackMap(uint32_t numMappedWords)

This assertion fails on x86-32 because MaxParams=1000, sizeof(void*)=4, yielding a value of 4016 here, which requires 12 bits. (By contrast, on ARM32 the max parameter size is 8 and it would not be a problem, and on 64-bit systems sizeof(void*)=8, which is also fine.) By intermingling externref and v128 parameters in the parameter area we should be able to force every parameter to use 16 bytes (natural alignment), thus the stack map will not cover some of the bytes of the frame, hence it may be that some values will not be traced and the pointers to those values will point to dead objects or unmapped space, and eventually to unrelated objects.

I'm guessing this is probably tricky to exploit but I don't know all the ramifications yet.

This has been a problem since SIMD landed in FF89.

Comment 1

[Lars T Hansen [:lth]]

I'm guessing this is not bad because there's a MOZ_RELEASE_ASSERT later that guards against overflow, so at worst we'll have a controlled crash.

Comment 2

[Lars T Hansen [:lth]]

TC that triggers that assertion:

var params = '';
for ( var i=0 ; i < 1000; i+= 2 ) {
    params += '(param v128) (param externref)'
}
var lastparam = 999;
new WebAssembly.Module(wasmTextToBinary(`
(module
  (func $f)
  (func ${params} (result externref)
    (call $f)
    (local.get ${lastparam})))`));

Comment 3

[Lars T Hansen [:lth]]

Attachment: Bug 1747870 - Account for large SIMD types in the stack map. r?jseward

Centrally this introduces a static_assert that the stack map's
frame-offset-from-top field can hold the values it needs to hold.

This static_assert turns out to fail on x86-32 with SIMD enabled, and
we can also provoke a release assert that guards against overflow with
the enclosed test case, which is valid code.

To fix this, the field must be expanded by one bit. It's an open
question whether we should do this only on x86-32, as here, or on all
platforms. Discuss.

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Account for large SIMD types in the stack map. r=jseward
https://hg.mozilla.org/integration/autoland/rev/9cc261380ffe0250dc825fbb8a990fd5ec93f1dchttps://hg.mozilla.org/integration/autoland/rev/9cc261380ffe0250dc825fbb8a990fd5ec93f1dc
https://hg.mozilla.org/mozilla-central/rev/9cc261380ffe

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance. It grafts cleanly as-landed. If we wanted this on ESR also, it'll require a rebased patch, but I'm not sure if this is worth taking there or not. Feel free to rebase and request approval if you think it is, though.

Comment 6

[Lars T Hansen [:lth]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #5)

Please nominate this for Beta approval when you get a chance. It grafts cleanly as-landed. If we wanted this on ESR also, it'll require a rebased patch, but I'm not sure if this is worth taking there or not. Feel free to rebase and request approval if you think it is, though.

Will do. As it is extremely obscure and will release-assert when it occurs, I think we'll leave ESR alone.

Comment 7

[Lars T Hansen [:lth]]

Comment on attachment 9257465 [details]
Bug 1747870 - Account for large SIMD types in the stack map. r?jseward

Beta/Release Uplift Approval Request

• User impact if declined: It is possible to create wasm content that crashes the tab (release-assert), leaving us open for a DOS mostly.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): We're just expanding a bitfield in order to make it large enough, and there are enough bits to take from.
• String changes made/needed:

Comment 8

[Donal Meehan [:dmeehan]]

Landed in 97.0b3
https://hg.mozilla.org/releases/mozilla-beta/rev/bc089adfb92f