Bug 173677 – Switch VMTraps to use halt instructions rather than breakpoint instructions

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

Swtich VMTraps to use halt instructions rather than breakpoint instructions

[Keith Miller]

Created attachment 313658 [details]
Patch

[JF Bastien]

Comment on attachment 313658 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        any exceptioning instruction we want. I went weth the halt instructions

"weth"

> Source/JavaScriptCore/ChangeLog:14
> +        find a single instruction on ARM that produces a SIGSEGV.

Can you document that x86 F1 and D6 were also treated as breakpoints by LLDB?

> Source/JavaScriptCore/ChangeLog:15
> +

On ARM64 all 0 and all 1 are guaranteed unallocated. They don't sigill?
Otherwise try one of the SYS instructions such as `AT S1E1R, X0` whose encoding is 0xd5087800. Address translation from user mode is definitely not something you're allowed to do :)

> Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:125
> +        { "FTL",      300, "--useConcurrentJIT=false --useLLInt=true --useJIT=true --useDFGJIT=true --useFTLJIT=true" },

wai?

> Source/JavaScriptCore/dfg/DFGJumpReplacement.cpp:46
> +#if CPU(X86) || CPU(X86_64) || CPU(ARM64)

Can you just use ENABLE_SIGNAL_BASED_VM_TRAPS here?

[JF Bastien]

Comment on attachment 313658 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:15
>> +
> 
> On ARM64 all 0 and all 1 are guaranteed unallocated. They don't sigill?
> Otherwise try one of the SYS instructions such as `AT S1E1R, X0` whose encoding is 0xd5087800. Address translation from user mode is definitely not something you're allowed to do :)

Oh sorry the goal was to segfault on ARM! Use `LDR SP, #0` which is a PC-relative load with offset zero. Since we do X-only that'll segfault. Encoding 0x1800001F.

[Mark Lam]

Comment on attachment 313658 [details]
Patch

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

r=me with some a suggestion and typo fixes.

>> Source/JavaScriptCore/ChangeLog:11
>> +        any exceptioning instruction we want. I went weth the halt instructions
> 
> "weth"

typo: /weth/with/

> Source/JavaScriptCore/ChangeLog:12
> +        since the naming was consistant and partly because I couldn't find

typo: /consistant/consistent/

> Source/JavaScriptCore/runtime/VMTraps.cpp:123
> +    // ARM does not have any instruction guarenteed to segmentation fault.

typo: /guarenteed/guaranteed/.

Since this code needs to match what is being done at in other code not obviously seen locally in this function, I think it's worth adding a comment here saying:
tryInstallTrapBreakpoints() will install "Halt" instructions (via MacroAssembler::replaceWithHalt()) that will trigger SIGILL on ARM64. On x86 and x86_64, the "Halt" instruction will trigger SIGSEGV.

[Mark Lam]

Comment on attachment 313658 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:15
>>> +
>> 
>> On ARM64 all 0 and all 1 are guaranteed unallocated. They don't sigill?
>> Otherwise try one of the SYS instructions such as `AT S1E1R, X0` whose encoding is 0xd5087800. Address translation from user mode is definitely not something you're allowed to do :)
> 
> Oh sorry the goal was to segfault on ARM! Use `LDR SP, #0` which is a PC-relative load with offset zero. Since we do X-only that'll segfault. Encoding 0x1800001F.

I like this.  It'll make the code more consistent if we always use Segv.  Plus it doesn't interfere with the SigillCrashAnalyzer.

>> Source/JavaScriptCore/dfg/DFGJumpReplacement.cpp:46
>> +#if CPU(X86) || CPU(X86_64) || CPU(ARM64)
> 
> Can you just use ENABLE_SIGNAL_BASED_VM_TRAPS here?

I agree.  Please do this.

[Mark Lam]

<rdar://problem/32178892>

[JF Bastien]

(In reply to Mark Lam from comment #5)
> Comment on attachment 313658 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313658&action=review
> 
> >>> Source/JavaScriptCore/ChangeLog:15
> >>> +
> >> 
> >> On ARM64 all 0 and all 1 are guaranteed unallocated. They don't sigill?
> >> Otherwise try one of the SYS instructions such as `AT S1E1R, X0` whose encoding is 0xd5087800. Address translation from user mode is definitely not something you're allowed to do :)
> > 
> > Oh sorry the goal was to segfault on ARM! Use `LDR SP, #0` which is a PC-relative load with offset zero. Since we do X-only that'll segfault. Encoding 0x1800001F.
> 
> I like this.  It'll make the code more consistent if we always use Segv. 
> Plus it doesn't interfere with the SigillCrashAnalyzer.

I talked to Keith on IRC yesterday and it sounds like there are a few issues with this. It can probably get resolved, but I'm OK checking this in without the change for now.


> >> Source/JavaScriptCore/dfg/DFGJumpReplacement.cpp:46
> >> +#if CPU(X86) || CPU(X86_64) || CPU(ARM64)
> > 
> > Can you just use ENABLE_SIGNAL_BASED_VM_TRAPS here?
> 
> I agree.  Please do this.

This would still be good to do though.

[JF Bastien]

Something else you can try:

1. Cause an alignment fault, but I think that'll be SIGBUS?
e.g. SP is always aligned in the ARM64 ABI and will fault when used and misaligned. Some accesses can do indexed write back... but the SP check occurs *before* the instruction (and it's aligned at that time, even with pre-index), so the fault would actually occur not on your instruction but on the next SP usage.

2. Try out BRK with other immediate. It's totally up to the kernel to decide what immediate means, I remember Jim a few years ago telling me about this... Maybe there's an encoding that'll cause what we want? It would be in the kernel source, or we can ask kernel folks (seems like a stretch for BRK to show up as segfault).

3. Branch to SP: 0xd61f43e0.

[Keith Miller]

(In reply to JF Bastien from comment #8)
> Something else you can try:
> 
> 1. Cause an alignment fault, but I think that'll be SIGBUS?
> e.g. SP is always aligned in the ARM64 ABI and will fault when used and
> misaligned. Some accesses can do indexed write back... but the SP check
> occurs *before* the instruction (and it's aligned at that time, even with
> pre-index), so the fault would actually occur not on your instruction but on
> the next SP usage.
> 
> 2. Try out BRK with other immediate. It's totally up to the kernel to decide
> what immediate means, I remember Jim a few years ago telling me about
> this... Maybe there's an encoding that'll cause what we want? It would be in
> the kernel source, or we can ask kernel folks (seems like a stretch for BRK
> to show up as segfault).
> 
> 3. Branch to SP: 0xd61f43e0.

Interesting! I'll try these.

[JF Bastien]

Someone on the internet suggested: d50b7420        dc      zva, x0
That's a data cache maintenance instruction, but I thought it was system only?
Person says it's user-accessible: https://twitter.com/translatedcode/status/878304596497707008

Choice of x0 is weird though, we're not guaranteed that zeroing out x0's virtual address cache line will be invalid. However, we know that LR will be invalid (as long as we follow the ABI and have a code location in LR).

So I'd try: 0xd50b743e

[JF Bastien]

(In reply to JF Bastien from comment #10)
> Someone on the internet suggested: d50b7420        dc      zva, x0
> That's a data cache maintenance instruction, but I thought it was system
> only?
> Person says it's user-accessible:
> https://twitter.com/translatedcode/status/878304596497707008
> 
> Choice of x0 is weird though, we're not guaranteed that zeroing out x0's
> virtual address cache line will be invalid. However, we know that LR will be
> invalid (as long as we follow the ABI and have a code location in LR).
> 
> So I'd try: 0xd50b743e

Oh the person said to do XZR instead, so 0xd50b743f. As long as that's not SP it'll be OK.

[JF Bastien]

OK so DC ZVA is totally accessible from user-mode! TIL.

It's encoded as follows:
op1	CRm	op2	<dc_op>
011	0100	001	ZVA

And CheckSystemAccess()'s pseudocode does the following:

    // ...

    case op1 of
        when '00x', '010'
            min_EL = EL1;
        when '011'
            min_EL = EL0; // <---- This.
        when '100'
            min_EL = EL2;
        when '101'
            UnallocatedEncoding();
        when '110'
            min_EL = EL3;
        when '111'
            min_EL = EL1;
            need_secure = TRUE;
    if UInt(PSTATE.EL) < UInt(min_EL) then
        UnallocatedEncoding();

    // ...

And X31 here is totally XZR because the manual doesn't say it's SP.

ARM's online docs say:

    The ARMv8-A architecture introduces a Data Cache Zero by
    Virtual Address (DC ZVA) instruction. This enables a block of
    64 bytes in memory, aligned to 64 bytes in size, to be set to
    zero. If the DC ZVA instruction misses in the cache, it
    clears main memory, without causing an L1 or L2 cache
    allocation.

And:

    The ability to preload the data cache with zero values using
    the DC ZVA instruction is new in ARMv8-A. Processors can
    operate significantly faster than external memory systems and
    it can sometimes take a long time to load a cache line from
    memory.

    Cache line zeroing behaves in a similar fashion to a
    prefetch, in that it is a way of hinting to the processor
    that certain addresses are likely to be used in the
    future. However, a zeroing operation can be much quicker as
    there is no need to wait for external memory accesses to
    complete. Instead of getting the actual data from memory read
    into the cache, you get cache lines filled with zeros. It
    enables hinting to the processor that the code completely
    overwrites the cache line contents, so there is no need for
    an initial read.

    Consider the case where you need a large temporary storage
    buffer or are initializing a new structure. You could have
    code simply start using the memory, or you could write code
    that prefetched it before using it. Both would use a lot of
    cycles and memory bandwidth in reading the initial contents
    to the cache. By using a cache zero option, you could
    potentially save this wasted bandwidth and execute the code
    faster.

[JF Bastien]

Yeah `dc zva, xzr` totally segfaults for me on iOS! So we know the kernel doesn't forbid its usage (which would sigill instead).

[Keith Miller]

Created attachment 313750 [details]
Patch

[Keith Miller]

Created attachment 313751 [details]
Patch

[JF Bastien]

Comment on attachment 313750 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:15
> +        which should trigger a segmentation fault.

Drop "should". It triggers it :)

> Source/JavaScriptCore/runtime/VMTraps.cpp:123
> +    // ARM does not have any instruction guarenteed to segmentation fault.

ARM bit wrong?

[Keith Miller]

Created attachment 313753 [details]
Patch for landing

[Mark Lam]

Comment on attachment 313753 [details]
Patch for landing

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

> Source/WTF/wtf/threads/Signals.cpp:167
> +    // If we wanted to distingish between SIGBUS and SIGSEGV for EXC_BAD_ACCESS on Darwin we could do:

typo: /distingish/distinguish/

> Source/WTF/wtf/threads/Signals.cpp:345
> -    struct sigaction& oldAction = oldActions[static_cast<size_t>(signal)];
> +    unsigned oldActionIndex = static_cast<size_t>(signal) + (sig == SIGBUS);
> +    struct sigaction& oldAction = oldActions[static_cast<size_t>(oldActionIndex)];

This is weird that you cast to a size_t only to be assigned to an unsigned.  And then you cast it to a size_t again when indexing into oldActions.  Let's just use size_t.

Also, this is kind of hacky to have Signal::BadAccess have an extra signal entry in oldActions, and to conveniently bump NumberOfSignals by 1 to make space for this.  This seems very fragile.  I think we should try to clean this up later and make it less hacky and fragile.

[Keith Miller]

Created attachment 313762 [details]
Patch for landing

[Build Bot]

Attachment 313762 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:122:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:124:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 313765 [details]
Patch for landing

[Keith Miller]

Created attachment 313766 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 313766 [details]
Patch for landing

Clearing flags on attachment: 313766

Committed r218782: <http://trac.webkit.org/changeset/218782>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.