RESOLVED FIXED 177745
[Linux] Enable Gigacage in x64 Linux environment
https://bugs.webkit.org/show_bug.cgi?id=177745
Summary [Linux] Enable Gigacage in x64 Linux environment
Yusuke Suzuki
Reported 2017-10-02 07:47:58 PDT
Proposal: Enabling Gigacage in Linux. Gigacage is implemented largely by fpizlo and it is a feature to limit the area referenced from a caged pointer. Once a pointer is caged, it can only reference to a specific area called Gigacage. This mechanism is implemented by `basePointer + (cagedPointer & cageMask)` This is good for security. Many exploit first attempts to replace ArrayBuffer's buffer pointer to a arbitrary area to modify arbitrary memory including JIT-executable ones. Caged pointer reduces the effectiveness of this attack by limitting the area that can be referenced from ArrayBuffer's caged pointer. Gigacage is now enabled in Darwin x64 environment. And Darwin + ARM64 work is ongoing (bug 177586).
Attachments
Patch (1.37 KB, patch)
2017-10-02 11:01 PDT, Yusuke Suzuki
no flags
Patch (3.10 KB, patch)
2017-10-24 11:56 PDT, Zan Dobersek
no flags
Patch for landing (3.16 KB, patch)
2017-10-24 23:08 PDT, Zan Dobersek
no flags
Yusuke Suzuki
Comment 1 2017-10-02 07:58:10 PDT
Note: Gigacage works fine on Linux as of https://trac.webkit.org/r221548 change.
Yusuke Suzuki
Comment 2 2017-10-02 11:01:57 PDT
Yusuke Suzuki
Comment 3 2017-10-02 11:55:12 PDT
Comment on attachment 322393 [details] Patch I've just discussed with Filip and decided to enable it :)
WebKit Commit Bot
Comment 4 2017-10-02 12:20:04 PDT
Comment on attachment 322393 [details] Patch Clearing flags on attachment: 322393 Committed r222731: <http://trac.webkit.org/changeset/222731>
WebKit Commit Bot
Comment 5 2017-10-02 12:20:05 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 6 2017-10-02 12:21:04 PDT
Charlie Turner
Comment 7 2017-10-04 04:35:28 PDT
I get a segfault about not being able to allocate the gigacage when I try to run under Valgrind, my workaround for now is to export Malloc=1 to disable bmalloc. Not sure if there's a better way to continue using bmalloc without a gigacage.
Michael Catanzaro
Comment 8 2017-10-04 06:40:25 PDT
I would not expect bmalloc to work well under valgrind anyway. Malloc=1 seems like an acceptable solution, right?
Yusuke Suzuki
Comment 9 2017-10-04 07:26:24 PDT
(In reply to Michael Catanzaro from comment #8) > I would not expect bmalloc to work well under valgrind anyway. Malloc=1 > seems like an acceptable solution, right? Oh, nice catch. I'm not sure malloc can work with valgrind without any annotations. Out of curiousity: does valgrind work with WASM Memory allocation? WASM memory also allocates large virtual memory region.
Charlie Turner
Comment 10 2017-10-04 07:56:01 PDT
(In reply to Michael Catanzaro from comment #8) > I would not expect bmalloc to work well under valgrind anyway. Malloc=1 > seems like an acceptable solution, right? Valgrind has never worked very well for me with WebKit anyway, but the Malloc=1 thing was not required before gigacages, I'm just checking it would be a sensible thing to suggest in our port documentation.
Carlos Alberto Lopez Perez
Comment 11 2017-10-04 10:06:58 PDT
(In reply to Charlie Turner from comment #10) > (In reply to Michael Catanzaro from comment #8) > > I would not expect bmalloc to work well under valgrind anyway. Malloc=1 > > seems like an acceptable solution, right? > > Valgrind has never worked very well for me with WebKit anyway, but the > Malloc=1 thing was not required before gigacages, I'm just checking it would > be a sensible thing to suggest in our port documentation. It will be, please free to add that info to the wiki (for example: https://trac.webkit.org/wiki/WebKitGTK/Debugging ) It may be also a good idea to check if "Tools/Scripts/run-webkit-tests --leaks" are still working for GTK+, and if not maybe propose a patch to set that env var from the webkitpy tooling before starting the tests.
Michael Catanzaro
Comment 12 2017-10-23 20:30:09 PDT
So unfortunately this broke core dumps. I know Gigacage is a great security feature, but it's not more important than being able to debug crashes. So I'm going to roll this out, sorry. :/ Specifically, Gigacage has resulted in massive core dumps that get truncated at whatever the max limit in /etc/systemd/coredump.conf, resulting in an unusable backtrace. By default the size limit 2 GB, but I tried raising it to 8 GB and the limit was still reached by every crash.
Michael Catanzaro
Comment 13 2017-10-23 20:32:32 PDT
Michael Catanzaro
Comment 14 2017-10-23 20:33:37 PDT
Reopening
Yusuke Suzuki
Comment 15 2017-10-24 02:21:09 PDT
(In reply to Michael Catanzaro from comment #12) > So unfortunately this broke core dumps. I know Gigacage is a great security > feature, but it's not more important than being able to debug crashes. So > I'm going to roll this out, sorry. :/ > > Specifically, Gigacage has resulted in massive core dumps that get truncated > at whatever the max limit in /etc/systemd/coredump.conf, resulting in an > unusable backtrace. By default the size limit 2 GB, but I tried raising it > to 8 GB and the limit was still reached by every crash. Hmm, this is really sad. OK, once we move Platform.h to bmalloc layer, I'll enable this on JSCOnly port.
Adrian Perez
Comment 16 2017-10-24 07:02:20 PDT
It turns out that the problem is not with gigacage: It's systemd-coredump. If the kernel is allowed to write the core dumps by itself by changing the value of “/proc/sys/kernel/core_pattern”, it works fine — systems which do not use systemd-coredump or have it disabled still get fine coredumps. My (unverified) suspicion is that sparse files with holes are being written by the kernel and systemd-coredump tries to write the full extents of the file and it is not detecting the holes when reading from the pipe (maybe it's not possible?). The net result is that systemd-coredump hits the core file size limit and truncates it. Unfortunately, the systemd developers have decided that truncated cores “are still useful” and not a bug (see https://github.com/systemd/systemd/issues/3883) I think gigacage should be re-enabled, and that systemd-coredump is buggy if it cannot handle core files that the kernel can generate without batting an eye.
Michael Catanzaro
Comment 17 2017-10-24 07:46:35 PDT
(In reply to Adrian Perez from comment #16) > My (unverified) suspicion is that sparse files with holes are being written > by the kernel and systemd-coredump tries to write the full extents of the > file and it is not detecting the holes when reading from the pipe (maybe > it's not possible?). The net result is that systemd-coredump hits the core > file size limit and truncates it. Unfortunately, the systemd developers > have decided that truncated cores “are still useful” and not a bug (see > https://github.com/systemd/systemd/issues/3883) Interesting. Alicia suggested we might try teaching coredumpctl to write out sparse files. Perhaps we should pursue this approach. > I think gigacage should be re-enabled, and that systemd-coredump is buggy > if it cannot handle core files that the kernel can generate without batting > an eye. We discussed this internally but did not come to an agreement.
Zan Dobersek
Comment 18 2017-10-24 11:56:04 PDT
Created attachment 324694 [details] Patch Yusuke, can you have a look?
Yusuke Suzuki
Comment 19 2017-10-24 12:07:31 PDT
Comment on attachment 324694 [details] Patch r=me. I don't like adding BOS(LINUX) much, but madvise is inherently platform dependent. And here, madvice needs to be called twice. So if we can consolidate these virtual memory operation difference in each platform into VMAllocate.h, I think it's OK.
Michael Catanzaro
Comment 20 2017-10-24 13:04:45 PDT
Comment on attachment 324694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324694&action=review Awesome! > Source/bmalloc/bmalloc/Gigacage.h:55 > +#if ((BOS(DARWIN) || BOS(LINUX)) && (BCPU(ARM64) || BCPU(X86_64))) Only one comment from me. This is not the condition we had before. Previously we had Gigacage enabled only for Linux x86_64, not for ARM64. You have a separate patch in bug #178130 to enable it there. (Yusuke, you'd probably be the best reviewer for that patch.) The condition we had here before was: #if (BOS(DARWIN) && (BCPU(ARM64) || BCPU(X86_64))) || (BOS(LINUX) && BCPU(X86_64))
Zan Dobersek
Comment 21 2017-10-24 23:05:44 PDT
Comment on attachment 324694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324694&action=review >> Source/bmalloc/bmalloc/Gigacage.h:55 >> +#if ((BOS(DARWIN) || BOS(LINUX)) && (BCPU(ARM64) || BCPU(X86_64))) > > Only one comment from me. This is not the condition we had before. Previously we had Gigacage enabled only for Linux x86_64, not for ARM64. You have a separate patch in bug #178130 to enable it there. (Yusuke, you'd probably be the best reviewer for that patch.) > > The condition we had here before was: > > #if (BOS(DARWIN) && (BCPU(ARM64) || BCPU(X86_64))) || (BOS(LINUX) && BCPU(X86_64)) Right, confused this with the work in #178130.
Zan Dobersek
Comment 22 2017-10-24 23:08:23 PDT
Created attachment 324797 [details] Patch for landing
Zan Dobersek
Comment 23 2017-10-24 23:55:38 PDT
Comment on attachment 324797 [details] Patch for landing Clearing flags on attachment: 324797 Committed r223950: <https://trac.webkit.org/changeset/223950>
Zan Dobersek
Comment 24 2017-10-24 23:55:43 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.