Closed Bug 1393011 Opened 2 years ago Closed 2 years ago

ARM JIT: audit usage of Instruction::next()

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: bbouvier, Assigned: sstangl)

References

Details

(Keywords: csectype-wildptr, sec-audit, Whiteboard: [post-critsmash-triage][adv-main59-])

User Story

:joleson did the arm64 audit. Still need to check out 32bit arm.

Attachments

(4 files, 3 obsolete files)

A few places in the code assume that in the instruction stream, when we have an Instruction pointer, the next instruction is located at this + 1:

http://searchfox.org/mozilla-central/source/js/src/jit/arm/Assembler-arm.cpp#3261
http://searchfox.org/mozilla-central/source/js/src/jit/arm/Assembler-arm.cpp#3268
http://searchfox.org/mozilla-central/source/js/src/jit/arm/Assembler-arm.cpp#3220
http://searchfox.org/mozilla-central/source/js/src/jit/arm/Assembler-arm.cpp#3223

The ARM64 backend does this kind of things too.

This is true... if and only if your ARMbuffer has been copied out to a *linear* buffer with executableCopy(). Otherwise, since the ARMbuffer instructions are laid out as a linked list of Slices (byte arrays), (this+1) might as well be something that's been malloc'ed otherwise, or the next Slice's header, as happened in bug 1392105.

So, to summarize:
- It is OK to use Instruction::next()/skipPool() if we're not in the masm (= after the code has been copied out from the masm).
- It is not OK to use those otherwise.

Pretty sure this is very unsafe: in the best case, we might crash when executableCopy() is called, in the worst case an attacker could write arbitrary instructions in the code stream and these would be later copied out and executed. I'm not too sure about the latter case: probably this would need knowing the Slice addresses etc to not overwrite their headers and destroy the linked list, but we'd better be careful here.

There's an AssemblerBufferInstIterator class in IonAssemblerBuffer.h which solves this exact problem (by jumping from one slice to the next when we're at a boundary). That's what we should use instead of the (this+1) calls in the functions mentioned above.

I think a full audit of the codebase should be done, to make sure the only users of next()/skipPool() are legit. Or, we could just wait for Cretonne...
Group: core-security
Nicolas, is your team aware of this?
Flags: needinfo?(nicolas.b.pierron)
Yes, this was a long standing issue from the way the ARM assembly got designed.

The problem of these pieces of code is that they are trying to match code which might be splat into 2 different buffers because of the insertion of a constant pool in the middle.

To solve these issue, we should use the instruction iterator which is made to follow unconditional jump added before splitting the generated code with a constant pool.

I think this issue is less of a problem for ARM, which is already suppose to use this iterator, or to prevent constant pool insertions in the matched pieces of code.  I do not know our ARM64 implementation to be able to say anything about it.
Flags: needinfo?(nicolas.b.pierron)
Jakob can you evaluate the risk here? I'm assuming this is wrt ARM64 baseline you worked on. Is this safe to leave till Cretonne can replace all our code generation for ARM in 2018?
Assignee: nobody → jolesen
Flags: needinfo?(jolesen)
Priority: -- → P1
In the ARM64 target, the function InstructionAtOffset() assumes a linear code layout. It is called from skipPool(), ToggleCall(), and CodeFromJump(). The skipPool() function is itself only called from ToggleCall() and CodeFromJump().

The CodeFromJump() function is called by the tracing phase of the garbage collector. AFAIK, it would have no reason to inquire about code stored in a MacroAssembler.

The ToggleCall() function is called from BaselineScript::toggleDebugTraps() which AFAIK does not operate on code stored in a MacroAssembler.

There are some additional calls to InstructionAtOffset() from Simulator-vixl.cpp which are harmless both because the simulator is only used for testing and because it works on copied-out code.

To summarize, these functions are intended to be used for inspecting and modifying code after is has been copied out of the MacroAssembler, and as far as I can tell, that is the only way they are used. I don't know that these functions are more or less dangerous than other functions called by the GC tracer and the debugger.
Flags: needinfo?(jolesen)
Note this *might* also be an issue in the ARM32 target, where the original bug 1392105 happened first.
Is 58 also affected? It is probably too late for a fix for 57. 
Jakob, are you still working on this?
There is nothing to fix, this bug is a speculative request for a code audit.

I have audited the ARM64 backend and found no defects. I am not sufficiently familiar with the ARM32 backend (which is entirely different) to audit that.

Naveed, if you believe there is something actionable for ARM32, would you pass it on to someone who knows the code?
Assignee: jolesen → nihsanullah
User Story: (updated)
Keywords: sec-highsec-audit
I'll do the 32-bit ARM audit. At first glance I believe it is safe, but I will do some cleanup to ensure that it's safe and make it more readable. I found some minor bugs already, but nothing that would cause crashes.
Assignee: nihsanullah → sstangl
Small renaming patch for clarity: skipPool() does not necessarily skip a pool, and it may skip things other than pools.
Attachment #8935564 - Flags: review?(bbouvier)
This fixes a but in PatchDataWithValueCheck(), caused by confusion around constant pools.

The function in question assumes that the two Instructions to be patched are located at `label.raw()` and `label.raw() + 4`, and flushes the instruction cache at those addresses.

However, internally, the ma_mov_patch() function knows that the "second" instruction may be located past a constant pool, and uses an iterator to skip the pool.

If this happens, we can think that we have overwritten an instruction, when the old version remains in cache. I believe this can occur in practice, but don't know if it's exploitable.
Attachment #8935566 - Flags: review?(bbouvier)
This patch fixes a bug in BufferInstructionIterator::maybeSkipAutomaticInstructions() where that function forgets that the underlying AssemblerBufferInstIterator, the base class of the BufferInstructionIterator, has an advance() method that already skips around constant pools by default.

The code here erroneously attempted to skip past constant pools /again/, which just winds up calling advance() to a random BufferOffset, resulting in the iterator pointing to a nonsensical memory address.

Given how wrong this code is, I suspect it doesn't actually occur in practice, since the users of BufferInstructionIterator are few. Nevertheless, this patch fixes it.
Attachment #8935568 - Flags: review?(nicolas.b.pierron)
This patch removes Instruction->next() and Instruction->maybeSkipAutomaticInstructions() and replaces their use with the InstructionIterator.

An explicit peekRaw() function is provided in lieu of manual pointer arithmetic, to make the behavior more auditable.

In addition, the interaction between next() and maybeSkipAutomaticInstructions() is clarified and defined non-recursively.

Your original description in Comment 1 of when it is acceptable to use the InstructionIterator is correct, and I didn't find any places where it was misused.

The ARM codebase still contains manual pointer arithmetic all over the place, but for the specific case of Instruction, InstructionIterator, and BufferInstructionIterator, I've found no bugs other than the ones fixed in previous patches.
Attachment #8935571 - Flags: review?(bbouvier)
Comment on attachment 8935564 [details] [diff] [review]
0001-Bug-1393011-Patch-1-4-Rename-skipPool-to-maybeSkipAu.patch

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

Better name, thanks.

::: js/src/jit/arm/Assembler-arm.h
@@ +2012,5 @@
>      Instruction* next();
>  
> +    // If the current instruction was automatically-inserted (pool guards and NOPs),
> +    // advance to the next instruction that was inserted intentionally.
> +    Instruction *maybeSkipAutomaticInstructions();

nit: Instruction* (star goes next to the type)
Attachment #8935564 - Flags: review?(bbouvier) → review+
Comment on attachment 8935566 [details] [diff] [review]
Part 2/4 - Fix bug in PatchDataWithValueCheck().

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

::: js/src/jit/arm/Assembler-arm.cpp
@@ +3166,5 @@
>  
> +#ifdef DEBUG
> +    {
> +        InstructionIterator iter(ptr);
> +        iter.maybeSkipAutomaticInstructions();

Would it make sense to hide the maybeSkipAutomaticInstructions() call under InstructionIterator's ctor itself, if all the users do this pattern of creating the iter then calling maybeSkip?
Attachment #8935566 - Flags: review?(bbouvier) → review+
Comment on attachment 8935571 [details] [diff] [review]
Part 4/4 - Use InstructionIterator

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

Looks good, thank you!

(please re-assign if the patch is substantially different after the proposed changes, but otherwise r=me)

::: js/src/jit/arm/Assembler-arm.cpp
@@ +3250,4 @@
>  }
>  
>  static bool
> +InstIsBNop(const InstructionIterator &iter)

nit: const InstructionIterator& iter

@@ +3268,5 @@
>      return offset.decode() == 4;
>  }
>  
>  static bool
> +InstIsBNop(const BufferInstructionIterator &iter)

ditto

also, any reason to not template this function over the parameter type? (EIBTI would be a good argument, but just checking in)

@@ +3294,4 @@
>      const PoolHeader* ph;
> +
> +    // Loop until an intentionally-placed instruction is found.
> +    while (1) {

nit: while (true)

Can we add an assertion that inst_ is within bounds? otherwise timeouts could hide in there and not be caught by fuzzers...

@@ +3298,5 @@
> +        if (InstIsGuard(cur(), &ph)) {
> +            // Don't skip a natural guard.
> +            if (ph->isNatural())
> +                return cur();
> +            moveTo(peekRaw(1) + ph->size());

This is entirely equivalent, but for uniformity with InstructionIterator::next below, can you peekRaw(1 + ph->size()) instead? (or change InstructionIterator::next so it does the same thing as here)

(see suggestion about moveTo in the other file; it also seems that the pattern is always moveTo(peekRaw()), so we could have a unique function that does both instead)

@@ -3340,4 @@
>  //    .word 0xffff8002  # bit 15 has no bearing on the returned value
>  //    0xdeadbeef
>  //    add r4, r4, r4  <= returned value
> -

I kinda like the blank line after multi-line comments...

@@ -3349,5 @@
> -    // If this is a guard, and the next instruction is a header, always work
> -    // around the pool. If it isn't a guard, then start looking ahead.
> -    if (InstIsGuard(this, &ph))
> -        return (ret + ph->size())->maybeSkipAutomaticInstructions();
> -    if (InstIsArtificialGuard(ret, &ph))

Trying to rephrase to make sure I understood correctly:
- in the new function, we won't enter the first `if`
- so we peek by one instruction and call maybeSkip
- which sees a guard that's not natural, peeks by 1 + ph->size() as here
- then keeps on looping (which is equivalent to the previous maybeSkip call)

If that's correct, then we can also remove the InstIsArtificialGuard function, used only here. (Otherwise, we need to update the new function)

::: js/src/jit/arm/Assembler-arm.h
@@ -2011,5 @@
> -    Instruction* next();
> -
> -    // If the current instruction was automatically-inserted (pool guards and NOPs),
> -    // advance to the next instruction that was inserted intentionally.
> -    Instruction *maybeSkipAutomaticInstructions();

Nice!

@@ +2278,5 @@
>      }
> +
> +  protected:
> +    // A simple wrapper around const_cast, for internal use.
> +    void moveTo(const Instruction* inst) {

since the pattern seems to be moveTo(peekRaw) all the time, and peekRaw returns a non-const, do we need the const_cast here? or could we just remove the function itself?
Attachment #8935571 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #15)
> 
> Can we add an assertion that inst_ is within bounds? otherwise timeouts
> could hide in there and not be caught by fuzzers...

We cannot, because the Iterator doesn't know the buffer length (it's mostly used for runtime instruction patching).

The good news is that the loop won't time out: the buffer position increases monotonically, so it will eventually return something or reference undefined memory. In practice, it will return quickly.

> (see suggestion about moveTo in the other file; it also seems that the
> pattern is always moveTo(peekRaw()), so we could have a unique function that
> does both instead)

I did it that way so there would only be one function in charge of selecting memory locations, for the benefit of auditing -- just peekRaw(). But it's not terrible if there's a second function.

> Trying to rephrase to make sure I understood correctly:
> - in the new function, we won't enter the first `if`
> - so we peek by one instruction and call maybeSkip
> - which sees a guard that's not natural, peeks by 1 + ph->size() as here
> - then keeps on looping (which is equivalent to the previous maybeSkip call)
> 
> If that's correct, then we can also remove the InstIsArtificialGuard
> function, used only here. (Otherwise, we need to update the new function)

That's correct. The behavior is handled by the maybe..(). I think you may have read the "from" version of the patch -- the patch does indeed remove InstIsArtificialGuard already :-)
Attachment #8935564 - Attachment is obsolete: true
Attachment #8935945 - Flags: review+
Attachment #8935566 - Attachment is obsolete: true
Attachment #8935946 - Flags: review+
Attachment #8935947 - Flags: review+
Attachment #8935571 - Attachment is obsolete: true
Attachment #8935568 - Flags: review?(nicolas.b.pierron) → review+
Keywords: checkin-needed
Group: javascript-core-security → core-security-release
Is this something that can ride the 59 train or were you thinking about uplifting it to Beta for 58?
Flags: needinfo?(sstangl)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> Is this something that can ride the 59 train or were you thinking about
> uplifting it to Beta for 58?

I think 59 is fine. I don't know that either of the two possible bugs identified can be reproduced in practice.
Flags: needinfo?(sstangl)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main59-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.