1437455 - Assertion failure: off.assigned() && offset >= 0 && unsigned(offset) < size(), at js/src/jit/shared/IonAssemblerBuffer.h:379

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 2b7d42d527af (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm, run with --fuzzing-safe --cpu-count=2 --disable-oom-functions --ion-offthread-compile=off --baseline-eager --ion-check-range-analysis --arm-sim-icache-checks --arm-hwcap=vfp):

function test(s) {
  return eval("line0 = Error.lineNumber\ndebugger\n" + s);
}
var g = newGlobal();
function repeat(s) {
  return Array(65 << 13).join("debugger;")
}
long_expr = repeat(g.ArrayBuffer)
long_throw_stmt = long_expr;
test(long_throw_stmt);


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x08512740 in js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst (this=0xffffa488, off=...) at js/src/jit/shared/IonAssemblerBuffer.h:379
#0  0x08512740 in js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst (this=0xffffa488, off=...) at js/src/jit/shared/IonAssemblerBuffer.h:379
#1  0x0850cd9f in js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInstOrNull (off=..., this=0xffffa488) at js/src/jit/shared/IonAssemblerBuffer.h:371
#2  js::jit::Assembler::writeInst (this=this@entry=0xffffa244, x=<optimized out>) at js/src/jit/arm/Assembler-arm.h:1436
#3  0x0850cefe in js::jit::Assembler::as_alu (this=0xffffa244, dest=..., src1=..., op2=..., op=js::jit::OpSub, s=js::jit::LeaveCC, c=js::jit::Assembler::Always) at js/src/jit/arm/Assembler-arm.cpp:1490
#4  0x085221fa in js::jit::MacroAssemblerARM::ma_alu (this=this@entry=0xffffa244, src1=src1@entry=..., imm=..., imm@entry=..., dest=..., scratch=..., op=js::jit::OpSub, c=js::jit::Assembler::Always, s=js::jit::LeaveCC) at js/src/jit/arm/MacroAssembler-arm.cpp:254
#5  0x0853b1c8 in js::jit::MacroAssemblerARM::ma_sub (c=js::jit::Assembler::Always, s=js::jit::LeaveCC, scratch=..., dest=..., imm=..., this=0xffffa244) at js/src/jit/arm/MacroAssembler-arm.cpp:652
#6  js::jit::MacroAssembler::subFromStackPtr (this=0xffffa244, imm32=...) at js/src/jit/arm/MacroAssembler-arm.cpp:4182
#7  0x0854303b in js::jit::MacroAssembler::reserveStack (amount=4, this=0xffffa244) at js/src/jit/MacroAssembler-inl.h:753
#8  js::jit::MacroAssembler::callWithABIPre (this=0xffffa244, stackAdjust=0xffff9dc4, callFromWasm=false) at js/src/jit/arm/MacroAssembler-arm.cpp:4608
#9  0x0843c9f6 in js::jit::MacroAssembler::callWithABINoProfiler (result=js::jit::MoveOp::GENERAL, check=js::jit::CheckUnsafeCallWithABI::DontCheckOther, fun=0xf6e430e4, this=0xffffa244) at js/src/jit/MacroAssembler.cpp:2960
#10 js::jit::MacroAssembler::callWithABI (this=this@entry=0xffffa244, fun=fun@entry=0xab579bf, check=js::jit::CheckUnsafeCallWithABI::DontCheckOther, result=js::jit::MoveOp::GENERAL) at js/src/jit/MacroAssembler-inl.h:109
#11 0x0843cb1f in js::jit::MacroAssembler::assumeUnreachable (this=0xffffa244, output=0x8bbaf9c "BaselineFrame shouldn't override pc when executing JIT code") at js/src/jit/MacroAssembler.cpp:1870
#12 0x085a62a6 in js::jit::BaselineCompilerShared::callVM (this=0xffffa238, fun=..., phase=js::jit::BaselineCompilerShared::POST_INITIALIZE) at js/src/jit/shared/BaselineCompiler-shared.cpp:73
#13 0x08b11ab5 in js::jit::BaselineCompiler::emit_JSOP_DEBUGGER (this=0xffffa238) at js/src/jit/BaselineCompiler.cpp:3923
#14 0x08b2ea79 in js::jit::BaselineCompiler::emitBody (this=0xffffa238) at js/src/jit/BaselineCompiler.cpp:1040
#15 0x08b3020c in js::jit::BaselineCompiler::compile (this=0xffffa238) at js/src/jit/BaselineCompiler.cpp:122
#16 0x08276740 in js::jit::BaselineCompile (cx=0xf6e1d800, script=0xf65711c8, forceDebugInstrumentation=false) at js/src/jit/BaselineJIT.cpp:256
#17 0x08278d7d in CanEnterBaselineJIT (cx=cx@entry=0xf6e1d800, script=script@entry=..., osrFrame=osrFrame@entry=0x0) at js/src/jit/BaselineJIT.cpp:300
#18 0x08278f17 in js::jit::CanEnterBaselineMethod (cx=0xf6e1d800, state=...) at js/src/jit/BaselineJIT.cpp:353
#19 0x083b0c2a in js::jit::MaybeEnterJit (cx=0xf6e1d800, state=...) at js/src/jit/Jit.cpp:151
#20 0x081a2f05 in js::RunScript (cx=0xf6e1d800, state=...) at js/src/vm/Interpreter.cpp:408
#21 0x081a5626 in js::ExecuteKernel (cx=0xf6e1d800, script=..., envChainArg=..., newTargetValue=..., evalInFrame=..., result=0xf67ffd6c) at js/src/vm/Interpreter.cpp:706
#22 0x081d7457 in EvalKernel (cx=0xf6e1d800, v=..., v@entry=..., evalType=evalType@entry=DIRECT_EVAL, caller=..., env=..., pc=0xf63d022b "{\001", vp=...) at js/src/builtin/Eval.cpp:324
#23 0x081d79d0 in js::DirectEval (cx=0xf6e1d800, v=..., vp=...) at js/src/builtin/Eval.cpp:434
#24 0x08281245 in js::jit::DoCallFallback (cx=0xf6e1d800, frame=0xf67ffdd0, stub_=0xf62ae0a8, argc=1, vp=0xf67ffd90, res=...) at js/src/jit/BaselineIC.cpp:2367
[...]
#55 main (argc=10, argv=0xffffcce4, envp=0xffffcd10) at js/src/shell/js.cpp:9317
eax	0x0	0
ebx	0x8dcfff4	148701172
ecx	0xf7d9f864	-136710044
edx	0x0	0
esi	0xffffa244	-23996
edi	0xffffa488	-23416
ebp	0xffff9c38	4294941752
esp	0xffff9c10	4294941712
eip	0x8512740 <js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst(js::jit::BufferOffset)+336>
=> 0x8512740 <js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst(js::jit::BufferOffset)+336>:	movl   $0x0,0x0
   0x851274a <js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst(js::jit::BufferOffset)+346>:	ud2


Marking s-s because the assertion looks bad. This one only seems to reproduce on ARM with --arm-hwcap=vfp in particular, otherwise I get an OOM.

Comment 1

[Daniel Veditz [:dveditz]]

Does this script crash Fennec? Or are there odd options used in your js shell run?

Comment 2

[Daniel Veditz [:dveditz]]

(debug fennec, obvs)

Comment 3

[Christian Holler (:decoder)]

As clarified over IRC, this test doesn't use any unusual shell flags that turn on experimental features or any such thing, with the exception of --arm-hwcap=vfp. I think this flag has to do with floating point support in the underlying CPU, so if it is required, only certain types of ARM CPUs will be affected.

Asking Lars on this because it is an ARM only bug.

Comment 4

[Lars T Hansen [:lth]]

Note, with --arm-hwcap=vfp you get a plain ARMv6 with VFP.  ARMv6 is tier-3 at best but really we've been ripping out some ARMv6 code from the JIT over the last year when it's in the way, so you're entirely on your own here.  For both --arm-hwcap and the environment variable ARMHWCAP you must specify a coherent set of flags to get a meaningful test run.  Adding "armv7" as a minimum is recommended; see js/src/jit/arm/Architecture-arm.cpp for more.  Maybe we should start printing a warning if the architecture selected is not at least ARMv7.

That said:

This looks like a standard C++ footgun; the "bufferSize" field in an assembler buffer is unsigned but the "offset" field of a BufferOffset is signed.  We should detect a buffer overflow (and probably go into an oom state) when the bufferSize field goes past INT_MAX but we do not.

I think that we fail at this assertion a little while after that field has overflowed, at which point we'll have > 2GB of instructions accumulated in a chunked buffer.  Since that chunked buffer would have to be flattened before we can run the code I think we should run out of memory even on a Linuxish system with 3GB of address space on 32-bit.  (Not sure what the case is on Android.)  So I don't think this is a critical security issue, since we only ship 32-bit with this code, the x64 product uses a separate buffer definition.

However it'll be a problem for ARM64 and MIPS64 when those ship and we need to fix this in the IonAssemblerBuffer code.  Looks like we can isolate this test to ensureSpace() and we can make newSlice() private to make sure the check is not bypassed.  We should not allocate a new slice if that would bring the total size past INT_MAX.

We should also assert in debug builds that a BufferOffset is never constructed with a negative value; that will catch errors sooner.

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1437455-limit-assembler-buffer.patch

Check for buffer sizes beyond INT32_MAX, and also assert that we don't construct  BufferOffset with offset values < 0.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8954351 [details] [diff] [review]
bug1437455-limit-assembler-buffer.patch

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

::: js/src/jit/shared/IonAssemblerBuffer.h
@@ +195,5 @@
>  
> +  private:
> +    Slice* newSlice(LifoAlloc& a) {
> +        // Clients of IonAssemblerBuffer can only handle a size up to INT_MAX.
> +        if (size() + sizeof(Slice) > INT32_MAX) {

nit: size() returns an "unsigned int", which would cause this condition to be always false on 32 bits platforms where int is defined as an int32. Instead, do:

    auto sz = CheckedInt32(size()) + sizeof(Slice);
    if (!sz.isValid()) {
        …

Comment 7

[Lars T Hansen [:lth]]

Really?  We know (I hope) that the value returned by size() should never be larger than INT32_MAX, because that's the invariant we're trying to maintain.  sizeof(Slice) is size_t, so also unsigned, but it is "small" compared to size() when size() nears the extreme.  So in the extreme case the sum should be an unsigned value somewhat but not greatly larger than INT32_MAX.  Since the lhs is unsigned INT32_MAX is converted to unsigned and the comparison is unsigned.

I'm happy to use CheckedInt32 but I'd like to understand why.

Comment 8

[Nicolas B. Pierron [:nbp]]

Scratch my last comment, the patch is correct because of the "unsigned" and "INT32_MAX" fits in the unsigned type.

Comment 9

[Lars T Hansen [:lth]]

I don't think it's worth the bother to uplift this, though it would not be risky to do so.

Comment 10

[Lars T Hansen [:lth]]

This landed, so I don't know why the annotation is not appearing here:

changeset:   405638:4c96fc4ec2b0
user:        Lars T Hansen <[e-mail]>
date:        Tue Feb 27 14:16:10 2018 +0100
summary:     Bug 1437455 - Guard against assembler buffer overrun.  r=nbp

Comment 11

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

> This landed, so I don't know why the annotation is not appearing here:
Pulsebot has no access to security bugs.

https://hg.mozilla.org/mozilla-central/rev/4c96fc4ec2b0(In reply to Lars T Hansen [:lth] from comment #10)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

AFAICT, this is a sec-high which affects more than just m-c. If that's true, it should have had sec-approval before landing. Also, we should probably consider uplifting this to 59, even though we're extremely low on time for doing so. Can you please do the requests ASAP, Lars?

Comment 13

[Lars T Hansen [:lth]]

I don't really understand why this should be sec-high or need an uplift.  It's not a buffer overflow, and so far as I can tell no invalid machine code can be created on any systems we support.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Dan, should we reconsider the sec rating of this bug?

Comment 15

[Lars T Hansen [:lth]]

(My apologies for not noticing the sec-high designation in the first place, obviously.)

I'll summarize the salient points quickly:

This is ARM-only at the moment as x86 and x64 do not use this code.

The original issue is that we hit an assertion that guards against a negative value in a BufferOffset variable; this is a type used by the JIT to represent offsets in an intermediate code buffer.  The negative value was caused by a quiet unsigned->signed conversion when copying the machine code buffer's unsigned length field into the BufferOffset.  The buffer can handle up to 4GB of data and does so correctly, but for sizes above 2GB the length cannot be represented correctly in a BufferOffset.

The fix is to prevent the quiet conversion and to OOM instead when buffer size exceeds INT32_MAX, at 2GB.

Note there is no buffer overrun per se, no data are written to any wrong addresses.

Since we have more than 2GB of intermediate machine code and a 32-bit ARM system has at most 4GB of virtual address space (in reality 2GB or 3GB) there is no way that we can produce final machine code, so no bad machine code will be produced.  In any case we cap the amount of machine code on 32-bit systems much lower, ISTR at 128MB.

The remaining issue then is whether the negative BufferOffset value can itself be used to force data (esp chosen data) to be written at the wrong address in memory during compilation, ie, outside the buffer, and whether this chosen data will survive the subsequent OOM handling (we'll OOM during final code pasteup).  I have not tried to perform an analysis of this.  Since the int32_t value, when converted back to uint32_t, is the "correct" offset when seen as unsigned, it is likely that most uses of a BufferOffset for indexing will be unproblematic even with the negative value.  So the risk here seems slight but without an exhaustive analysis it's impossible to say there's no risk.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Thanks for the extra info. Sounds like this can safely ride the 60 train, though I'll leave the NI on Dan in case he feels strongly otherwise.