Bug 177745 – [Linux] Enable Gigacage in x64 Linux environment

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

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).

[Yusuke Suzuki]

Note: Gigacage works fine on Linux as of https://trac.webkit.org/r221548 change.

[Yusuke Suzuki]

Created attachment 322393 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 322393 [details]
Patch

I've just discussed with Filip and decided to enable it :)

[WebKit Commit Bot]

Comment on attachment 322393 [details]
Patch

Clearing flags on attachment: 322393

Committed r222731: <http://trac.webkit.org/changeset/222731>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34773148>

[Charlie Turner]

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]

I would not expect bmalloc to work well under valgrind anyway. Malloc=1 seems like an acceptable solution, right?

[Yusuke Suzuki]

(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]

(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]

(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]

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]

Committed r223877: <https://trac.webkit.org/changeset/223877>

[Michael Catanzaro]

Reopening

[Yusuke Suzuki]

(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]

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]

(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]

Created attachment 324694 [details]
Patch

Yusuke, can you have a look?

[Yusuke Suzuki]

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 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 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]

Created attachment 324797 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 324797 [details]
Patch for landing

Clearing flags on attachment: 324797

Committed r223950: <https://trac.webkit.org/changeset/223950>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.