780936 - IonMonkey: Crash at weird address and !exploitable shows an Exploitable Data Execution Prevention Violation

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: fragile testcase

The attached testcase crashes js debug and opt shells on IonMonkey changeset b2361e15b665 with --ion-eager and -a at 0x0000000001cfa3e7 on Windows 7.

0:000:x86> !exploitable -v
HostMachine\HostUser
Executing Processor Architecture is x86
Debuggee is in User Mode
Debuggee is a live user mode debugging session on the local machine
Event Type: Exception
Exception Faulting Address: 0x1cfa3e7
First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005)
Exception Sub-Type: Data Execution Protection (DEP) Violation

Exception Hash (Major/Minor): 0x6e05193a.0x7505193a

Stack Trace:
Unknown
Instruction Address: 0x0000000001cfa3e7

Description: Data Execution Prevention Violation
Short Description: DEPViolation
Exploitability Classification: EXPLOITABLE
Recommended Bug Title: Exploitable - Data Execution Prevention Violation starting at Unknown Symbol @ 0x0000000001cfa3e7 (Hash=0x6e05193a.0x7505193a)

User mode DEP access violations are exploitable.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

There is no stack even in Mac 64-bit shells:

opt:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000103514510
0x0000000103514510 in ?? ()
(gdb) bt
#0  0x0000000103514510 in ?? ()
Cannot access memory at address 0x103514510
(gdb) x/i $pc
0x103514510:	Cannot access memory at address 0x103514510
(gdb)

debug:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000104067720
0x0000000104067720 in ?? ()
(gdb) bt
#0  0x0000000104067720 in ?? ()
Cannot access memory at address 0x104067720
(gdb)

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I manually bisected this after the vague crash pattern confused autobisect.

The first bad revision is:
changeset:   99584:fdc72b8c249c
user:        Jan de Mooij
date:        Fri Jul 06 11:17:34 2012 +0200
summary:     Bug 767419 - Support idempotent GetProperty ICs. r=dvander,bhackett

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Valgrind stack

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: slightly more reduced testcase

(thanks Nicolas for helping me out a little more here)


Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x00000001034518fc
0x00000001034518fc in ?? ()
(gdb) b EnterIon
Breakpoint 1 at 0x1005751cc: file /Users/skywalker/Desktop/jsfunfuzz-ionmonkey-g7q_zK-b2361e15b665-103396/compilePath/js/src/ion/Ion.cpp, line 1096.
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: /Users/skywalker/Desktop/jsfunfuzz-ionmonkey-g7q_zK-b2361e15b665-103396/js-dbg-64-ionmonkey-darwin --ion-eager -a 169.js

Breakpoint 1, EnterIon (cx=0x101115c20, fp=0x1012750b0, jitcode=0x101699f20) at /Users/skywalker/Desktop/jsfunfuzz-ionmonkey-g7q_zK-b2361e15b665-103396/compilePath/js/src/ion/Ion.cpp:1096
1096	    JS_CHECK_RECURSION(cx, return IonExec_Error);
(gdb) disable 1
(gdb) c
Continuing.
fuzzSeed: 104908815

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x00000001034518fc
0x00000001034518fc in ?? ()
(gdb) call js_DumpBacktrace(0x101115c20)
#0            0x0   169.js:717 (0x102318970 @ 18)
#1            0x0   169.js:712 (0x1023188c8 @ 91)
#2            0x0   169.js:727 (0x102318c10 @ 15)
#3            0x0   169.js:717 (0x102318970 @ 18)
#4            0x0   169.js:712 (0x1023188c8 @ 91)
#5            0x0   169.js:723 (0x102318b68 @ 7)
#6            0x0   169.js:712 (0x1023188c8 @ 91)
#7            0x0   169.js:720 (0x102318a18 @ 7)
#8            0x0   169.js:712 (0x1023188c8 @ 91)
#9            0x0   169.js:727 (0x102318c10 @ 15)
#10            0x0   169.js:717 (0x102318970 @ 18)
#11            0x0   169.js:712 (0x1023188c8 @ 91)
#12            0x0   169.js:720 (0x102318a18 @ 7)
#13            0x0   169.js:712 (0x1023188c8 @ 91)
#14            0x0   169.js:720 (0x102318a18 @ 7)
#15            0x0   169.js:712 (0x1023188c8 @ 91)
#16            0x0   169.js:723 (0x102318b68 @ 7)
#17            0x0   169.js:712 (0x1023188c8 @ 91)
#18            0x0   169.js:727 (0x102318c10 @ 15)
#19            0x0   169.js:717 (0x102318970 @ 18)
#20            0x0   169.js:712 (0x1023188c8 @ 91)
#21            0x0   169.js:720 (0x102318a18 @ 7)
#22            0x0   169.js:712 (0x1023188c8 @ 91)
#23            0x0   169.js:720 (0x102318a18 @ 7)
#24            0x0   169.js:712 (0x1023188c8 @ 91)
#25            0x0   169.js:720 (0x102318a18 @ 7)
#26            0x0   169.js:712 (0x1023188c8 @ 91)
#27            0x0   169.js:727 (0x102318c10 @ 15)
#28            0x0   169.js:717 (0x102318970 @ 18)
#29            0x0   169.js:712 (0x1023188c8 @ 91)
#30    0x101275770   169.js:723 (0x102318b68 @ 7)
#31    0x1012756f0   169.js:712 (0x1023188c8 @ 91)
#32    0x101275660   169.js:727 (0x102318c10 @ 15)
#33    0x1012755e0   169.js:717 (0x102318970 @ 18)
#34    0x101275558   169.js:712 (0x1023188c8 @ 91)
#35    0x1012754c8   169.js:720 (0x102318a18 @ 7)
#36    0x101275448   169.js:712 (0x1023188c8 @ 91)
#37    0x1012753b8   169.js:706 (0x102318778 @ 48)
#38            0x0   169.js:675 (0x102318388 @ 42)
#39    0x101275288   169.js:283 (0x102315580 @ 67)
#40    0x1012751e0   169.js:229 (0x102315190 @ 157)
#41    0x101275150   169.js:655 (0x102318040 @ 28)
#42    0x1012750c0   169.js:220 (0x102315190 @ 40)
#43    0x101275030   169.js:732 (0x1023070e8 @ 2344)
warning: Unable to restore previously selected frame.
(gdb)

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Aggresively reduced testcase

Comment 6

[Nicolas B. Pierron [:nbp]]

This issue sounds like a marking issue of the Invalidation path.  What happens after we are compiling multiple times and did multiple bailouts:

- call & compile.
- invalidate all ion code.
- return into ThunkToInterpreter.
- return in the argument rectifier.
- return in the Ion Code of the parent frame and follow the jump added by the invalidation.
- SEGV because the memory is no longer readable.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: Invalidation, Add space to avoid damaging other return paths.

CodeGenerator::visitCallGeneric can generate the following sequence of instruction
   0x00007ffff7f8af60:  …
   0x00007ffff7f8af63:  callq  0x7ffff7f88008
   0x00007ffff7f8af68:  jmpq   0x7ffff7f8af6f
   0x00007ffff7f8af6d:  callq  *%rbx
   0x00007ffff7f8af6f:  add    $0x8,%rsp
   0x00007ffff7f8af73:  …

The issue being that during an invalidation the invalidation path assume that the call-site will never be used and used the call-site to store the displacement to the invalidation epilogue.  Unfortunately, when both calls are used and that the script got invalidated both call-site are patched with 4 bytes of data.  This patch damage the jump located at 0x00007ffff7f8af68 and makes it jump in unmapped memory.

The current patch just add extra unused instructions after the jump between the 2 calls such as patch of the second call-site does not damage the jump needed to join the OSIPoint located after the LCallGeneric.

Still, I wonder where this call-site patch is needed, any ideas?

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 651093 [details] [diff] [review]
Invalidation, Add space to avoid damaging other return paths.

Good catch... these are tricky and thus I'm a little reluctant to spot fix it here, in this way.

It seems like instead, calls which may be invalidated should store the current assembler position. Then, at each call, we should either:

(1) Automatically insert nops as padding if the delta isn't high enough, or
(2) Assert that there is enough space and include a function to calculate the correct padding and insert nops.

I actually rather like #2 as then we could replace the logic at the end of CG*::generateInvalidateEpilogue(), and avoid nops being inserted after the "bind(&rejoin)".

Comment 10

[Nicolas B. Pierron [:nbp]]

Attachment: Move code and fix bug.

Comment 11

[Nicolas B. Pierron [:nbp]]

This bug should not longer reproduce with Bug 784568's patch.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #11)
> This bug should not longer reproduce with Bug 784568's patch.

Verifying that this does not reproduce anymore on Windows 7 with IonMonkey changeset f9ff9c554d4b.

Resolving FIXED by bug 784568.

The aggressively-reduced testcase in comment 5 can be checked in eventually... when it's safe. Setting the in-testsuite? flag.

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 654036 [details] [diff] [review]
Move code and fix bug.

Nicolas says this is obsolete, canceling review.

Comment 16

[bhavana bajaj [:bajaj]]

IonMonkey bug,marking esr10 unaffected