Bug 201570 – Harden protection of the Gigacage Config parameters.

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Mark Lam]

<rdar://problem/55134229>

[Mark Lam]

Created attachment 378259 [details]
proposed patch.

[Saam Barati]

Comment on attachment 378259 [details]
proposed patch.

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

> Source/bmalloc/ChangeLog:23
> +        3. Use a storeStore fence in freezeGigacageConfig() and permanentlyFreezeGigacageConfig()
> +           to ensure that values in the Config has been written out before we freeze it.

this doesn't sound necessary. The CPU would be wrong if it moved such a store below whatever the kernel is doing

[Saam Barati]

Comment on attachment 378259 [details]
proposed patch.

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

> Source/bmalloc/bmalloc/Gigacage.cpp:86
> +    std::atomic_thread_fence(std::memory_order_seq_cst);

yeah this seems dubious and unneeded.

[Saam Barati]

Comment on attachment 378259 [details]
proposed patch.

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

> Source/bmalloc/bmalloc/Gigacage.cpp:121
> +    result = vm_protect(mach_task_self(), reinterpret_cast<vm_address_t>(&g_gigacageConfig), configSizeToProtect, true, VM_PROT_READ);

can you name "true" here?

[Mark Lam]

Comment on attachment 378259 [details]
proposed patch.

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

>> Source/bmalloc/ChangeLog:23
>> +           to ensure that values in the Config has been written out before we freeze it.
> 
> this doesn't sound necessary. The CPU would be wrong if it moved such a store below whatever the kernel is doing

I was thinking that, but wanted to be get 2nd opinions to be doubly sure.  Will remove.

>> Source/bmalloc/bmalloc/Gigacage.cpp:121
>> +    result = vm_protect(mach_task_self(), reinterpret_cast<vm_address_t>(&g_gigacageConfig), configSizeToProtect, true, VM_PROT_READ);
> 
> can you name "true" here?

Sure.  I'll use an enum with descriptive names like AllowPermissionChangesAfterThis and DisallowPermissionChangesAfterThis.

[Saam Barati]

Comment on attachment 378259 [details]
proposed patch.

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

> Source/bmalloc/ChangeLog:20
> +           On OS(DARWIN), this is made possible by using vm_protect with a true
> +           set_maximum argument.  We also add a g_gigacageConfig.isPermanentlyFrozen flag
> +           that we assert.

you should explain what this is for DARWIN

> Source/bmalloc/bmalloc/Gigacage.cpp:90
> +    result = vm_protect(mach_task_self(), reinterpret_cast<vm_address_t>(&g_gigacageConfig), configSizeToProtect, false, VM_PROT_READ);

let's name "false" here

> Source/bmalloc/bmalloc/Gigacage.cpp:116
> +    std::atomic_thread_fence(std::memory_order_seq_cst);

also is unneeded. Is is not expected that multiple threads can call this at the same time, right?

[Mark Lam]

Comment on attachment 378259 [details]
proposed patch.

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

Thanks for the review.

>> Source/bmalloc/ChangeLog:20
>> +           that we assert.
> 
> you should explain what this is for DARWIN

Will do.

>> Source/bmalloc/bmalloc/Gigacage.cpp:90
>> +    result = vm_protect(mach_task_self(), reinterpret_cast<vm_address_t>(&g_gigacageConfig), configSizeToProtect, false, VM_PROT_READ);
> 
> let's name "false" here

Done.

>> Source/bmalloc/bmalloc/Gigacage.cpp:116
>> +    std::atomic_thread_fence(std::memory_order_seq_cst);
> 
> also is unneeded. Is is not expected that multiple threads can call this at the same time, right?

Removed.  It's expected that only the main thread should call forbidDisablingPrimitiveGigacage() (and therefore, permanentlyFreezeGigacageConfig()).  We're not expecting other threads to call this.

[Mark Lam]

Created attachment 378264 [details]
patch for landing.

[Mark Lam]

Created attachment 378265 [details]
patch for landing.

[Mark Lam]

Created attachment 378276 [details]
patch for landing.

[Mark Lam]

Landed in r249608: <http://trac.webkit.org/r249608>.

[Mark Lam]

FYI, fixed disabled-gigacage-* failures in https://bugs.webkit.org/show_bug.cgi?id=201577.

[Mark Lam]

(In reply to Mark Lam from comment #12)
> FYI, fixed disabled-gigacage-* failures in
> https://bugs.webkit.org/show_bug.cgi?id=201577.

Correction: fixed disabled-gigacage-* test failures in https://bugs.webkit.org/show_bug.cgi?id=201579.

[Chris Dumez]

I am getting a lot of crashes running the layout tests:
Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x00000000bbadbeef
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Segmentation fault: 11
Termination Reason:    Namespace SIGNAL, Code 0xb
Terminating Process:   exc handler [9131]

Application Specific Information:
This process is running with libgmalloc.dylib (GuardMalloc) which may have forced the crash due to a memory access error.
 

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000740256780 Gigacage::forbidDisablingPrimitiveGigacage() + 32
1   com.apple.WebKit              	0x000000010a5c987e WebKit::WebProcess::WebProcess() + 1246 (WebProcess.cpp:224)
2   com.apple.WebKit              	0x000000010a5c9395 WebKit::WebProcess::WebProcess() + 21 (WebProcess.cpp:224)
3   com.apple.WebKit              	0x000000010a5c933b WebKit::WebProcess::singleton() + 43 (WebProcess.cpp:176)
4   com.apple.WebKit              	0x000000010a39c811 void WebKit::initializeAuxiliaryProcess<WebKit::WebProcess>(WebKit::AuxiliaryProcessInitializationParameters&&) + 17 (XPCServiceEntryPoint.h:73)
5   com.apple.WebKit              	0x000000010a399689 void WebKit::XPCServiceInitializer<WebKit::WebProcess, WebKit::XPCServiceInitializerDelegate>(WTF::OSObjectPtr<NSObject<OS_xpc_object>*>, NSObject<OS_xpc_object>*, NSObject<OS_xpc_object>*) + 921 (XPCServiceEntryPoint.h:128)
6   com.apple.WebKit              	0x000000010a3992b1 WebContentServiceInitializer + 81 (WebContentServiceEntryPoint.mm:53)
7   com.apple.WebKit              	0x0000000109d3d00a invocation function for block in WebKit::XPCServiceEventHandler(NSObject<OS_xpc_object>*) + 650 (XPCServiceMain.mm:79)
8   libxpc.dylib                  	0x00007fff650d9078 _xpc_connection_call_event_handler + 56
9   libxpc.dylib                  	0x00007fff650d7268 _xpc_connection_mach_event + 927
10  libdispatch.dylib             	0x00007fff64e3e57e _dispatch_client_callout4 + 9
11  libdispatch.dylib             	0x00007fff64e53aeb _dispatch_mach_msg_invoke + 435
12  libdispatch.dylib             	0x00007fff64e43950 _dispatch_lane_serial_drain + 263
13  libdispatch.dylib             	0x00007fff64e5463e _dispatch_mach_invoke + 481
14  libdispatch.dylib             	0x00007fff64e49a91 _dispatch_main_queue_callback_4CF + 783
15  com.apple.CoreFoundation      	0x00007fff2db5bd00 __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
16  com.apple.CoreFoundation      	0x00007fff2db5b44a __CFRunLoopRun + 2370
17  com.apple.CoreFoundation      	0x00007fff2db5a883 CFRunLoopRunSpecific + 499
18  com.apple.Foundation          	0x00007fff301e9e2d -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 212
19  com.apple.Foundation          	0x00007fff301e9d46 -[NSRunLoop(NSRunLoop) run] + 76
20  libxpc.dylib                  	0x00007fff650f49d6 _xpc_objc_main.cold.4 + 49
21  libxpc.dylib                  	0x00007fff650dd7e1 _xpc_objc_main + 559
22  libxpc.dylib                  	0x00007fff650dd2fc xpc_main + 377
23  com.apple.WebKit              	0x0000000109d32cea WebKit::XPCServiceMain(int, char const**) + 1322 (XPCServiceMain.mm:147)
24  com.apple.WebKit              	0x000000010accc12b WKXPCServiceMain + 27 (WKMain.mm:33)
25  com.apple.WebKit.WebContent   	0x00000001095baeb2 main + 34 (AuxiliaryProcessMain.cpp:30)
26  libdyld.dylib                 	0x00007fff64e8e2a5 start + 1

[Chris Dumez]

Unfortunately, EWS does not run WK2 debug.

[Mark Lam]

(In reply to Chris Dumez from comment #14)
> I am getting a lot of crashes running the layout tests:
...
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore      	0x0000000740256780
> Gigacage::forbidDisablingPrimitiveGigacage() + 32
> 1   com.apple.WebKit              	0x000000010a5c987e
> WebKit::WebProcess::WebProcess() + 1246 (WebProcess.cpp:224)
...


I'm investigating.

[Mark Lam]

Created attachment 378306 [details]
follow up fix patch.

[Mark Lam]

Thanks for the review.  Follow up fix landed in r249621: <http://trac.webkit.org/r249621>.