Bug 201570 - Harden protection of the Gigacage Config parameters.
Summary: Harden protection of the Gigacage Config parameters.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 201521
Blocks: 201577
  Show dependency treegraph
 
Reported: 2019-09-06 17:16 PDT by Mark Lam
Modified: 2019-09-07 18:19 PDT (History)
14 users (show)

See Also:


Attachments
proposed patch. (18.97 KB, patch)
2019-09-06 18:06 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff
patch for landing. (19.08 KB, patch)
2019-09-06 19:01 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch for landing. (19.08 KB, patch)
2019-09-06 19:02 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch for landing. (19.08 KB, patch)
2019-09-06 21:04 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
follow up fix patch. (1.94 KB, patch)
2019-09-07 17:17 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-09-06 17:16:22 PDT
<rdar://problem/55134229>
Comment 1 Mark Lam 2019-09-06 18:06:07 PDT
Created attachment 378259 [details]
proposed patch.
Comment 2 Saam Barati 2019-09-06 18:12:36 PDT
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
Comment 3 Saam Barati 2019-09-06 18:14:45 PDT
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.
Comment 4 Saam Barati 2019-09-06 18:16:21 PDT
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?
Comment 5 Mark Lam 2019-09-06 18:21:52 PDT
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.
Comment 6 Saam Barati 2019-09-06 18:37:47 PDT
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?
Comment 7 Mark Lam 2019-09-06 18:50:17 PDT
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.
Comment 8 Mark Lam 2019-09-06 19:01:15 PDT
Created attachment 378264 [details]
patch for landing.
Comment 9 Mark Lam 2019-09-06 19:02:52 PDT
Created attachment 378265 [details]
patch for landing.
Comment 10 Mark Lam 2019-09-06 21:04:17 PDT
Created attachment 378276 [details]
patch for landing.
Comment 11 Mark Lam 2019-09-06 22:42:51 PDT
Landed in r249608: <http://trac.webkit.org/r249608>.
Comment 12 Mark Lam 2019-09-07 07:55:23 PDT
FYI, fixed disabled-gigacage-* failures in https://bugs.webkit.org/show_bug.cgi?id=201577.
Comment 13 Mark Lam 2019-09-07 07:57:37 PDT
(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.
Comment 14 Chris Dumez 2019-09-07 15:13:11 PDT
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
Comment 15 Chris Dumez 2019-09-07 15:44:02 PDT
Unfortunately, EWS does not run WK2 debug.
Comment 16 Mark Lam 2019-09-07 16:08:39 PDT
(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.
Comment 17 Mark Lam 2019-09-07 17:17:25 PDT
Created attachment 378306 [details]
follow up fix patch.
Comment 18 Mark Lam 2019-09-07 18:19:56 PDT
Thanks for the review.  Follow up fix landed in r249621: <http://trac.webkit.org/r249621>.