WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
173677
Switch VMTraps to use halt instructions rather than breakpoint instructions
https://bugs.webkit.org/show_bug.cgi?id=173677
Summary
Switch VMTraps to use halt instructions rather than breakpoint instructions
Keith Miller
Reported
2017-06-21 16:10:37 PDT
Swtich VMTraps to use halt instructions rather than breakpoint instructions
Attachments
Patch
(21.36 KB, patch)
2017-06-22 14:07 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(27.45 KB, patch)
2017-06-23 15:00 PDT
,
Keith Miller
jfbastien
: review+
Details
Formatted Diff
Diff
Patch
(26.76 KB, patch)
2017-06-23 15:02 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch for landing
(26.74 KB, patch)
2017-06-23 15:06 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch for landing
(28.65 KB, patch)
2017-06-23 17:32 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch for landing
(28.23 KB, patch)
2017-06-23 19:08 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch for landing
(27.78 KB, patch)
2017-06-23 19:11 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Keith Miller
Comment 1
2017-06-22 14:07:55 PDT
Created
attachment 313658
[details]
Patch
JF Bastien
Comment 2
2017-06-22 14:28:20 PDT
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 3
2017-06-22 14:36:11 PDT
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 4
2017-06-22 14:36:40 PDT
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 5
2017-06-22 14:47:14 PDT
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
Comment 6
2017-06-23 09:35:45 PDT
<
rdar://problem/32178892
>
JF Bastien
Comment 7
2017-06-23 09:37:10 PDT
(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
Comment 8
2017-06-23 10:02:50 PDT
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
Comment 9
2017-06-23 10:22:42 PDT
(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
Comment 10
2017-06-23 10:54:16 PDT
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
Comment 11
2017-06-23 10:55:32 PDT
(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
Comment 12
2017-06-23 11:10:30 PDT
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
Comment 13
2017-06-23 11:22:02 PDT
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
Comment 14
2017-06-23 15:00:05 PDT
Created
attachment 313750
[details]
Patch
Keith Miller
Comment 15
2017-06-23 15:02:55 PDT
Created
attachment 313751
[details]
Patch
JF Bastien
Comment 16
2017-06-23 15:04:49 PDT
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
Comment 17
2017-06-23 15:06:34 PDT
Created
attachment 313753
[details]
Patch for landing
Mark Lam
Comment 18
2017-06-23 15:27:52 PDT
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
Comment 19
2017-06-23 17:32:46 PDT
Created
attachment 313762
[details]
Patch for landing
Build Bot
Comment 20
2017-06-23 17:36:18 PDT
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
Comment 21
2017-06-23 19:08:44 PDT
Created
attachment 313765
[details]
Patch for landing
Keith Miller
Comment 22
2017-06-23 19:11:27 PDT
Created
attachment 313766
[details]
Patch for landing
WebKit Commit Bot
Comment 23
2017-06-23 19:54:06 PDT
Comment on
attachment 313766
[details]
Patch for landing Clearing flags on attachment: 313766 Committed
r218782
: <
http://trac.webkit.org/changeset/218782
>
WebKit Commit Bot
Comment 24
2017-06-23 19:54:08 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.
Top of Page
Format For Printing
XML
Clone This Bug