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).
Note: Gigacage works fine on Linux as of https://trac.webkit.org/r221548 change.
Created attachment 322393 [details] Patch
Comment on attachment 322393 [details] Patch I've just discussed with Filip and decided to enable it :)
Comment on attachment 322393 [details] Patch Clearing flags on attachment: 322393 Committed r222731: <http://trac.webkit.org/changeset/222731>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34773148>
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.
I would not expect bmalloc to work well under valgrind anyway. Malloc=1 seems like an acceptable solution, right?
(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.
(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.
(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.
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.
Committed r223877: <https://trac.webkit.org/changeset/223877>
Reopening
(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.
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.
(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.
Created attachment 324694 [details] Patch Yusuke, can you have a look?
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.
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))
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.
Created attachment 324797 [details] Patch for landing
Comment on attachment 324797 [details] Patch for landing Clearing flags on attachment: 324797 Committed r223950: <https://trac.webkit.org/changeset/223950>