1406883 - wasm: Make the coercion of raw value to MutableHandleValue explicit in CoerceInPlace calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P2)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Keeping hidden just in case, but I think it is pretty safe.

What happens here is that:

- wasm calls into a (JS) jit function.
- the return value might have been rooted by the jit function, if it needed to.
- there is a small range of instructions during in which the value isn't rooted, but no GC can happen in this range: an interrupt check would see we're not in wasm *function* code (this is an exit to C++ or jit code), and thus not trigger a gc.
- the next time a GC could happen is in the CoerceInPlace functions, under the ToInt32/ToNumber functions.

Before this patch, we converted a raw Value* into a MutableHandleValue via an implicit reinterpret_cast (generated code passes a Value* argument, C++ code reads it as a MutableHandleValue). Now with this patch, the coercion and rooting are made explicit. So there are no changes in functionality.

For what it's worth, this has been present since the beginning of asm.js/wasm, and while the fuzzers have found issues in this code path in the past, nothing was related to rooting of this value. Considering this fact, as well as the above reasoning, I think this has always been safe. So this patch is rather about documentation, but I would like to make sure the comment is correctly worded and that no dragons are hiding.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: rooting.patch

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8916553 [details] [diff] [review]
rooting.patch

Review of attachment 8916553 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for making things more explicit.

::: js/src/wasm/WasmBuiltins.cpp
@@ +296,5 @@
>  {
>      JSContext* cx = TlsContext.get();
>  
> +    MutableHandleValue val(MutableHandleValue::fromMarkedLocation(rawVal));
> +    RootedValue rootedVal(cx, val);

This isn't doing what I think you think it's doing.

This copies the contents of val into rootedVal and roots that - but it doesn't root val.

Probably we need to make val a RootedValue and use that for the calculations, and then ensure that we always write over rawVal before returning.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8916553 [details] [diff] [review]
rooting.patch

Review of attachment 8916553 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/wasm/WasmBuiltins.cpp
@@ +296,5 @@
>  {
>      JSContext* cx = TlsContext.get();
>  
> +    MutableHandleValue val(MutableHandleValue::fromMarkedLocation(rawVal));
> +    RootedValue rootedVal(cx, val);

Agreed with Jon that this isn't right as is, but Jon, can't we just do:

  RootedValue rootedVal(cx, *unrootedVal)
  ...  ToInt32
  *unrootedVal = Int32Value(i32);

so that, even though unrootedVal can end up holding a stale Value if ToInt32() GCs, we just overwrite that value with an Int32Value at the end so it doesn't matter?

::: js/src/wasm/WasmStubs.cpp
@@ +774,5 @@
> +    // In all the code paths from here:
> +    // - either the value is unboxed because it was a primitive and we don't
> +    //   need to worry about rooting anymore.
> +    // - or the value needs to be rooted, but nothing can cause a GC from
> +    //   here up to the next rooting (happening in the CoerceInPlace functions).

"the next rooting" isn't a totally defined phrase, so how about "but nothing can cause a GC between here and CoerceInPlace, which roots before coercing to a primitive"

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: rooting.patch

Thanks for the reviews! I think you are both suggesting the same thing, so wrote my understanding of this.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8916906 [details] [diff] [review]
rooting.patch

Review of attachment 8916906 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this i what I was suggesting.  Looks good.  Just one thing...

::: js/src/wasm/WasmBuiltins.cpp
@@ +301,2 @@
>      if (!ToInt32(cx, val, &i32))
>          return false;

Can the contents of rawVal get used if we return false here?  It might be worth overwriting it on failure too, since it could contain a stale pointer at this point.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8916906 [details] [diff] [review]
rooting.patch

Review of attachment 8916906 [details] [diff] [review]:
-----------------------------------------------------------------

Ah right, we were saying the same thing :)

::: js/src/wasm/WasmStubs.cpp
@@ +769,5 @@
>  
>      AssertStackAlignment(masm, JitStackAlignment, sizeOfRetAddr);
>      masm.callJitNoProfiler(callee);
>  
> +    // Note that there might be a marked GC value in the JSReturnOperand now.

nit: instead of "marked GC value" how about "a GC thing" (as in v.isGCThing())?

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: rooting.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Almost sure it's impossible. The cause of this being sec-sensitive is that a value is left unrooted for some time. But it only flows into calls that can't GC before getting rooted again (from CoerceInPlace_ToInt32 it's unrooted until ToNumberSlow [1]). In my opinion, we could just let this ride the trains, since it hasn't ever been noticed and it is highly likely to be not exploitable.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes, comments do.

Which older supported branches are affected by this flaw?
Probably there since jit calls from wasm have been optimized (can't find the bug number, but all branches are affected).

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backporting patches could be made and wouldn't be too risky as well.

How likely is this patch to cause regressions; how much testing does it need?
It passes the full test suite locally and shouldn't need much more testing.

[0] http://searchfox.org/mozilla-central/source/js/src/wasm/WasmBuiltins.cpp#295 (val is the unrooted value)
[1] http://searchfox.org/mozilla-central/source/js/src/jsnum.cpp#1585

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8917374 [details] [diff] [review]
rooting.patch

Clearing sec-approval since Dan made this a sec-audit issue after looking at it. Just check it into m-c.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f430beef5228ce7fe416da5a55c8abe6a6682b55

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f430beef5228