Bug 167239 – ArityFixup should adjust SP first

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kamil Frankowicz]

Created attachment 299345 [details]
POC to heap out of bounds write (jsc)

Affected SVN revision: 210958

To reproduce the problem:
./jsc jsc_oobw_llint_entry.js

GDB Backtrace:

#0  0x00007ffff767a830 in llint_entry ()
   from XYZ/WebKitBuild/Release/lib/libJavaScriptCore.so.1
#1  0x00007ffff767fe91 in llint_entry ()
   from XYZ/WebKitBuild/Release/lib/libJavaScriptCore.so.1
#2  0x00007ffff767945f in vmEntryToJavaScript ()
   from XYZ/WebKitBuild/Release/lib/libJavaScriptCore.so.1
#3  0x00007ffff7603aae in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) ()
   from XYZ/WebKitBuild/Release/lib/libJavaScriptCore.so.1
#4  0x00007ffff75d512f in JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) ()
   from XYZ/WebKitBuild/Release/lib/libJavaScriptCore.so.1
#5  0x00007ffff77cfd5a in JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) () from XYZ/WebKitBuild/Release/lib/libJavaScriptCore.so.1
#6  0x000000000040de87 in runJSC(JSC::VM*, CommandLine) ()
#7  0x000000000040c997 in jscmain(int, char**) ()
#8  0x000000000040c867 in main ()
#9  0x00007ffff3bdd830 in __libc_start_main (main=0x40c850 <main>, argc=0x2, argv=0x7fffffffdcd8, 
    init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffdcc8)
    at ../csu/libc-start.c:291
#10 0x000000000040ad79 in _start ()

Regards,
Kamil Frankowicz

[Alexey Proskuryakov]

I cannot reproduce on Mac with ASan build of r211025.

[Kamil Frankowicz]

I tried on r211363 + ASAN and problem still exist.

My configuration:
CC = afl-clang-fast (Clang 3.9.1)
CXX = afl-clang-fast++ (Clang 3.9.1)
OS = Xubuntu 16.04 x64

[Yusuke Suzuki]

Reproduced in JSCOnly port.
I'm now investigating this.
Basically the problem is too many arguments.
More simplified POC for this issue.

var args = "y,".repeat(80000);
var g = Function(args, "return 0");
g();

[Yusuke Suzuki]

crashed position is functionArityCheck's copyLoop.

[Yusuke Suzuki]

Current my guess is that soft stack height check is failed in Linux due to the different stack height in each platform.

[Yusuke Suzuki]

soft stack height check should pass.
The problem is the arity check slow path function does not return failure.

[Yusuke Suzuki]

OK, it seems that Linux softStackLimit is incorrect.

[Yusuke Suzuki]

ok, it seems that LLInt code bug. quad signed words are handled with long size ops.

[Yusuke Suzuki]

OK, finally found the issue.
The problem is, in Linux, we use pthread_attr_getstack to obtain stack base and size.
But it does not return the correct value if it is called on the main thread.

[Yusuke Suzuki]

Aha! I've got the reason of this bug.
It is not related to loop operation and stack limits.
It is related to x64 red zone!

Now, we perform copy loop. It extends the C stack.
But we accidentally perform this operation before extending sp.
Logically it is ok. But this means that you now touch beyond the red zone of the stack!

[Yusuke Suzuki]

(In reply to comment #10)
> Aha! I've got the reason of this bug.
> It is not related to loop operation and stack limits.
> It is related to x64 red zone!
> 
> Now, we perform copy loop. It extends the C stack.
> But we accidentally perform this operation before extending sp.
> Logically it is ok. But this means that you now touch beyond the red zone of
> the stack!

I guess that some crashes I saw when enabling sampling profilers are also related to the red zone.
At that time, signals frequently comes. This breaks the stack that is beyond the red zone.

[Yusuke Suzuki]

Created attachment 300252 [details]
Patch

[Michael Saboff]

Comment on attachment 300252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300252&action=review

r=me
How does adjusting the stack pointer adjust the red zone?

> Source/JavaScriptCore/ChangeLog:8
> +        Arity fixup extends C stack and copy/fill the stack with

There is only one stack.  How about "... extends the stack and copy/fills the"?

> Source/JavaScriptCore/ChangeLog:10
> +        the values. At that time, we accidentally read/write stacks
> +        before extending the stack pointer. As a result, we touch

How about replacing "stacks before extending the stack pointer" with "stack space behind the stack pointer"

> Source/JavaScriptCore/ChangeLog:12
> +        are unsafe, especially in Linux. This patch extends the stack

How about changing "extends" to "changes"?

[Mark Lam]

Comment on attachment 300252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300252&action=review

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:445
> +    // Adjust call frame register and stack pointer to account for missing args
> +    // We need to extend sp first before starting the copy/fill loops since it
> +    // may exceed the x64 red zone.

I think this comment is not entirely accurate and does not explain the core issue.  I think the issue is that the OS expects any space below the sp to be unused, and will allocate that region to be used by the interrupt / signal stack.  If we do not adjust the sp before filling in that region with content, the content may get trashed by an interrupt.  As a result, we need to adjust the sp first.  Incidentally, the reason that the interrupt handler can just use that region below the sp is because we are supposed to reserve an amount of memory (the red zone size) at the bottom of the stack that user code is not supposed to use.

Can you update the comments to clarify the issue please before landing?  Thanks.

[Michael Catanzaro]

Thanks Yusuke!

[Build Bot]

Comment on attachment 300252 [details]
Patch

Attachment 300252 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2983099

New failing tests:
css3/filters/backdrop/dynamic-with-clip-path.html

[Build Bot]

Created attachment 300300 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Yusuke Suzuki]

Comment on attachment 300252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300252&action=review

>> Source/JavaScriptCore/ChangeLog:8
>> +        Arity fixup extends C stack and copy/fill the stack with
> 
> There is only one stack.  How about "... extends the stack and copy/fills the"?

OK, we dropped the JS stack for JIT / LLInt! Changed.

>> Source/JavaScriptCore/ChangeLog:10
>> +        before extending the stack pointer. As a result, we touch
> 
> How about replacing "stacks before extending the stack pointer" with "stack space behind the stack pointer"

Sounds nice. I merged the suggestion from Mark. Then, I think rephasing it "stack space below the stack pointer" is nice.

>> Source/JavaScriptCore/ChangeLog:12
>> +        are unsafe, especially in Linux. This patch extends the stack
> 
> How about changing "extends" to "changes"?

Sounds nice. Fixed.

>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:445
>> +    // may exceed the x64 red zone.
> 
> I think this comment is not entirely accurate and does not explain the core issue.  I think the issue is that the OS expects any space below the sp to be unused, and will allocate that region to be used by the interrupt / signal stack.  If we do not adjust the sp before filling in that region with content, the content may get trashed by an interrupt.  As a result, we need to adjust the sp first.  Incidentally, the reason that the interrupt handler can just use that region below the sp is because we are supposed to reserve an amount of memory (the red zone size) at the bottom of the stack that user code is not supposed to use.
> 
> Can you update the comments to clarify the issue please before landing?  Thanks.

Yup. The problem is SP is not updated before modifying the area that is lower than SP.
In that case, OS can break these areas since OS thinks it is unused.
In Linux port, we can encounter this bug more frequently since we use signals for suspend and resume the threads (SIGUSR2).
In addition, the Linux kernel could not populate the stack pages if the area is beyond the SP and the red zone.
This is the direct reason of this issue.
When touching such a page, the kernel does not populate the backing physical pages and report SEGV instead.

I'll update these comments.

[Yusuke Suzuki]

Committed r211479: <http://trac.webkit.org/changeset/211479>

[Radar WebKit Bug Importer]

<rdar://problem/30311611>

[Kamil Frankowicz]

This is CVE-2017-5949.