487049 - TraceMonkey: Some corner-case ARM branches may be invalid.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacob Bramley [:jbramley]]

It is fairly typical in the ARM nanojit to see this kind of construct for branch construction:

offs = PC_OFFSET_FROM(...);
if (isS24(offs))
{
   underrunProtect(4);
   // Recalculate offs as underrunProtect may have moved _nIns to a different page.
   offs = PC_OFFSET_FROM(...);
}

There's a flaw in the above logic: If underrunProtect moves _nIns to a new page, there's a small change that the new page may be out of range of a branch instruction (as checked by isS24) whilst the old page was fine. This will cause execution to jump to some weird place in memory and start executing whatever's there.

Is this a remote code execution risk? Well, it is, but it'd be almost impossible to make use of it.

A better solution (which is actually used in a couple of places) is to do underrunProtect(4) before checking the offset.


I'm working on something in this area for bug 486639 so I'll create a patch for this bug whilst I'm at it.

Comment 1

[Jacob Bramley [:jbramley]]

Assembler::BL is one of the afflicted methods, but this is removed by the patch in bug 487607 so I have marked this bug as depending on bug 487607 and will produce an appropriate patch to fix any remaining instances of this bug.

Comment 2

[Jacob Bramley [:jbramley]]

All other methods that I can see implement valid logic, so this bug will be fixed once BL is removed (as a result of bug 487607).

Comment 3

[Jacob Bramley [:jbramley]]

Bug 487607 was just fixed, so I'm closing this one.