WebKit Bugzilla
Attachment 373409 Details for
Bug 199372
: PACCage should first cage leaving PAC bits intact then authenticate
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-199372-20190703132433.patch (text/plain), 18.83 KB, created by
Keith Miller
on 2019-07-03 13:24:34 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Keith Miller
Created:
2019-07-03 13:24:34 PDT
Size:
18.83 KB
patch
obsolete
>Subversion Revision: 247100 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index 22da981539f33bb4216c43be8deb9419d39cdf5b..e2a364e1e8f3651b416340e506ebe7998e6f8642 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,63 @@ >+2019-07-03 Keith Miller <keith_miller@apple.com> >+ >+ PACCage should first cage leaving PAC bits intact then authenticate >+ https://bugs.webkit.org/show_bug.cgi?id=199372 >+ >+ Reviewed by Saam Barati. >+ >+ This ordering prevents someone from taking a signed pointer from >+ outside the gigacage and using it in a struct that expects a caged >+ pointer. Previously, the PACCaging just double checked that the PAC >+ bits were valid for the original pointer. >+ >+ >+ +---------------------------+ >+ | | | | >+ | "PAC" | "base" | "offset" +----+ >+ | | | | | >+ +---------------------------+ | Caging >+ | | >+ | | >+ | v >+ | +---------------------------+ >+ | | | | | >+ | Bit Merge | 00000 | base | "offset" | >+ | | | | | >+ | +---------------------------+ >+ | | >+ | | >+ v | Bit Merge >+ +---------------------------+ | >+ | | | | | >+ | "PAC" | base | "offset" +<--------+ >+ | | | | >+ +---------------------------+ >+ | >+ | >+ | Authenticate >+ | >+ v >+ +---------------------------+ >+ | | | | >+ | Auth | base | "offset" | >+ | | | | >+ +---------------------------+ >+ >+ The above ascii art graph shows how the PACCage system works. The >+ key take away is that even if someone passes in a valid, signed >+ pointer outside the cage it will still fail to authenticate as the >+ "base" bits will change before authentication. >+ >+ >+ * assembler/MacroAssemblerARM64E.h: >+ * assembler/testmasm.cpp: >+ (JSC::testCagePreservesPACFailureBit): >+ * ftl/FTLLowerDFGToB3.cpp: >+ (JSC::FTL::DFG::LowerDFGToB3::caged): >+ * jit/AssemblyHelpers.h: >+ (JSC::AssemblyHelpers::cageConditionally): >+ * llint/LowLevelInterpreter64.asm: >+ > 2019-07-03 Paulo Matos <pmatos@igalia.com> > > Refactoring of architectural Register Information >diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog >index 3aa26cb2c8f9cc7356b727fb286584f7d6950a0c..88f81b741cf7d96e5565e75db033c6a3238a56a5 100644 >--- a/Source/WTF/ChangeLog >+++ b/Source/WTF/ChangeLog >@@ -1,3 +1,15 @@ >+2019-07-03 Keith Miller <keith_miller@apple.com> >+ >+ PACCage should first cage leaving PAC bits intact then authenticate >+ https://bugs.webkit.org/show_bug.cgi?id=199372 >+ >+ Reviewed by Saam Barati. >+ >+ * wtf/CagedPtr.h: >+ (WTF::CagedPtr::get const): >+ (WTF::CagedPtr::getMayBeNull const): >+ (WTF::CagedPtr::mergePointers): >+ > 2019-07-03 Ryan Haddad <ryanhaddad@apple.com> > > Unreviewed, rolling out r246616. >diff --git a/Source/bmalloc/ChangeLog b/Source/bmalloc/ChangeLog >index 5731f60cca5353188d18a4fa96f27fe3d86dd322..0fac1aaa33ed704eb4aab955dfa6a6a2c0039c0d 100644 >--- a/Source/bmalloc/ChangeLog >+++ b/Source/bmalloc/ChangeLog >@@ -1,3 +1,13 @@ >+2019-07-03 Keith Miller <keith_miller@apple.com> >+ >+ PACCage should first cage leaving PAC bits intact then authenticate >+ https://bugs.webkit.org/show_bug.cgi?id=199372 >+ >+ Reviewed by Saam Barati. >+ >+ * bmalloc/ProcessCheck.mm: >+ (bmalloc::shouldProcessUnconditionallyUseBmalloc): >+ > 2019-07-02 Commit Queue <commit-queue@webkit.org> > > Unreviewed, rolling out r247041. >diff --git a/Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h b/Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h >index fd7eec808551bdf7287afc1b1ec453f2f82082d2..9c6e775e517445efcb5cb8c078b79b05a08918ae 100644 >--- a/Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h >+++ b/Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h >@@ -40,6 +40,7 @@ using Assembler = TARGET_ASSEMBLER; > class MacroAssemblerARM64E : public MacroAssemblerARM64 { > public: > static constexpr unsigned numberOfPACBits = 25; >+ static constexpr uintptr_t nonPACBitsMask = (1ull << (64 - numberOfPACBits)) - 1; > > ALWAYS_INLINE void tagReturnAddress() > { >diff --git a/Source/JavaScriptCore/assembler/testmasm.cpp b/Source/JavaScriptCore/assembler/testmasm.cpp >index 1050b39983e4e65052824c04f0bb1254a313e2c8..3cbaed8a4751de591ac8d04a579c7afffb13747d 100644 >--- a/Source/JavaScriptCore/assembler/testmasm.cpp >+++ b/Source/JavaScriptCore/assembler/testmasm.cpp >@@ -1015,6 +1015,8 @@ void testMoveDoubleConditionally64() > > static void testCagePreservesPACFailureBit() > { >+#if GIGACAGE_ENABLED >+ ASSERT(!Gigacage::isDisablingPrimitiveGigacageDisabled()); > auto cage = compile([] (CCallHelpers& jit) { > jit.emitFunctionPrologue(); > jit.cageConditionally(Gigacage::Primitive, GPRInfo::argumentGPR0, GPRInfo::argumentGPR1, GPRInfo::argumentGPR2); >@@ -1025,10 +1027,15 @@ static void testCagePreservesPACFailureBit() > > void* ptr = Gigacage::tryMalloc(Gigacage::Primitive, 1); > void* taggedPtr = tagArrayPtr(ptr, 1); >- dataLogLn("starting test"); >+ ASSERT(hasOneBitSet(Gigacage::size(Gigacage::Primitive) << 2)); >+ void* notCagedPtr = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(ptr) + (Gigacage::size(Gigacage::Primitive) << 2)); >+ CHECK_NOT_EQ(Gigacage::caged(Gigacage::Primitive, notCagedPtr), notCagedPtr); >+ void* taggedNotCagedPtr = tagArrayPtr(notCagedPtr, 1); >+ > if (isARM64E()) { > // FIXME: This won't work if authentication failures trap but I don't know how to test for that right now. > CHECK_NOT_EQ(invoke<void*>(cage, taggedPtr, 2), ptr); >+ CHECK_EQ(invoke<void*>(cage, taggedNotCagedPtr, 1), untagArrayPtr(taggedPtr, 2)); > } else > CHECK_EQ(invoke<void*>(cage, taggedPtr, 2), ptr); > >@@ -1042,16 +1049,17 @@ static void testCagePreservesPACFailureBit() > jit.ret(); > }); > >+ CHECK_EQ(invoke<void*>(cageWithoutAuthentication, taggedPtr), taggedPtr); > if (isARM64E()) { > // FIXME: This won't work if authentication failures trap but I don't know how to test for that right now. >- CHECK_NOT_EQ(invoke<void*>(cageWithoutAuthentication, untagArrayPtr(taggedPtr, 2)), ptr); >- } else >- CHECK_EQ(invoke<void*>(cageWithoutAuthentication, untagArrayPtr(taggedPtr, 2)), ptr); >- >- CHECK_EQ(untagArrayPtr(taggedPtr, 1), ptr); >- CHECK_EQ(invoke<void*>(cageWithoutAuthentication, untagArrayPtr(taggedPtr, 1)), ptr); >+ CHECK_NOT_EQ(invoke<void*>(cageWithoutAuthentication, taggedNotCagedPtr), taggedNotCagedPtr); >+ CHECK_NOT_EQ(untagArrayPtr(invoke<void*>(cageWithoutAuthentication, taggedNotCagedPtr), 1), notCagedPtr); >+ CHECK_NOT_EQ(invoke<void*>(cageWithoutAuthentication, taggedNotCagedPtr), taggedPtr); >+ CHECK_NOT_EQ(untagArrayPtr(invoke<void*>(cageWithoutAuthentication, taggedNotCagedPtr), 1), ptr); >+ } > > Gigacage::free(Gigacage::Primitive, ptr); >+#endif > } > > #define RUN(test) do { \ >diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp >index 55a2897e0052912ae64949b9f633727a4c384138..1af74148c95200fdadfd0d674c6261fbef4b8d93 100644 >--- a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp >+++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp >@@ -14177,18 +14177,7 @@ private: > > LValue caged(Gigacage::Kind kind, LValue ptr, LValue base) > { >-#if CPU(ARM64E) >- if (kind == Gigacage::Primitive) { >- LValue size = m_out.load32(base, m_heaps.JSArrayBufferView_length); >- ptr = untagArrayPtr(ptr, size); >- } >-#else >- UNUSED_PARAM(kind); >- UNUSED_PARAM(base); >-#endif >- > #if GIGACAGE_ENABLED >- UNUSED_PARAM(base); > if (!Gigacage::isEnabled(kind)) > return ptr; > >@@ -14206,7 +14195,7 @@ private: > LValue result = m_out.add(masked, basePtr); > > #if CPU(ARM64E) >- { >+ if (kind == Gigacage::Primitive) { > PatchpointValue* merge = m_out.patchpoint(pointerType()); > merge->append(result, B3::ValueRep(B3::ValueRep::SomeLateRegister)); > merge->appendSomeRegister(ptr); >@@ -14214,9 +14203,12 @@ private: > jit.move(params[2].gpr(), params[0].gpr()); > jit.bitFieldInsert64(params[1].gpr(), 0, 64 - MacroAssembler::numberOfPACBits, params[0].gpr()); > }); >- result = merge; >+ >+ LValue size = m_out.load32(base, m_heaps.JSArrayBufferView_length); >+ result = untagArrayPtr(merge, size); > } >-#endif >+#endif // CPU(ARM64E) >+ > // Make sure that B3 doesn't try to do smart reassociation of these pointer bits. > // FIXME: In an ideal world, B3 would not do harmful reassociations, and if it did, it would be able > // to undo them during constant hoisting and regalloc. As it stands, if you remove this then Octane >@@ -14230,6 +14222,9 @@ private: > // https://bugs.webkit.org/show_bug.cgi?id=175493 > return m_out.opaque(result); > #endif >+ >+ UNUSED_PARAM(kind); >+ UNUSED_PARAM(base); > return ptr; > } > >diff --git a/Source/JavaScriptCore/jit/AssemblyHelpers.h b/Source/JavaScriptCore/jit/AssemblyHelpers.h >index a77d43a501086730d7dc9985f42db5219a2f9259..3eead2d8e06ef5c689234857cb2a0c0d9e5eafd6 100644 >--- a/Source/JavaScriptCore/jit/AssemblyHelpers.h >+++ b/Source/JavaScriptCore/jit/AssemblyHelpers.h >@@ -1586,42 +1586,41 @@ public: > // length may be the same register as scratch. > void cageConditionally(Gigacage::Kind kind, GPRReg storage, GPRReg length, GPRReg scratch) > { >+#if GIGACAGE_ENABLED >+ if (Gigacage::isEnabled(kind)) { >+ if (kind != Gigacage::Primitive || Gigacage::isDisablingPrimitiveGigacageDisabled()) >+ cageWithoutUntagging(kind, storage); >+ else { >+#if CPU(ARM64E) >+ if (length == scratch) >+ scratch = getCachedMemoryTempRegisterIDAndInvalidate(); >+#endif >+ loadPtr(&Gigacage::basePtr(kind), scratch); >+ Jump done = branchTest64(Zero, scratch); >+#if CPU(ARM64E) >+ GPRReg tempReg = getCachedDataTempRegisterIDAndInvalidate(); >+ move(storage, tempReg); >+ ASSERT(LogicalImmediate::create64(Gigacage::mask(kind)).isValid()); >+ andPtr(TrustedImmPtr(Gigacage::mask(kind)), tempReg); >+ addPtr(scratch, tempReg); >+ bitFieldInsert64(tempReg, 0, 64 - numberOfPACBits, storage); >+#else >+ andPtr(TrustedImmPtr(Gigacage::mask(kind)), storage); >+ addPtr(scratch, storage); >+#endif // CPU(ARM64E) >+ done.link(this); >+ } >+ } >+#endif >+ > #if CPU(ARM64E) > if (kind == Gigacage::Primitive) > untagArrayPtr(length, storage); >-#else >+#endif > UNUSED_PARAM(kind); > UNUSED_PARAM(storage); > UNUSED_PARAM(length); >-#endif >- >-#if GIGACAGE_ENABLED >- if (!Gigacage::isEnabled(kind)) >- return; >- >- if (kind != Gigacage::Primitive || Gigacage::isDisablingPrimitiveGigacageDisabled()) >- cageWithoutUntagging(kind, storage); >- else { >- loadPtr(&Gigacage::basePtr(kind), scratch); >- Jump done = branchTestPtr(Zero, scratch); >-#if CPU(ARM64E) >- auto tempReg = getCachedMemoryTempRegisterIDAndInvalidate(); >- move(storage, tempReg); >- andPtr(TrustedImmPtr(Gigacage::mask(kind)), tempReg); >- addPtr(scratch, tempReg); >- bitFieldInsert64(tempReg, 0, 64 - numberOfPACBits, storage); >-#else >- andPtr(TrustedImmPtr(Gigacage::mask(kind)), storage); >- addPtr(scratch, storage); >-#endif >- done.link(this); >- >- >- } >-#else > UNUSED_PARAM(scratch); >-#endif >- > } > > void emitComputeButterflyIndexingMask(GPRReg vectorLengthGPR, GPRReg scratchGPR, GPRReg resultGPR) >diff --git a/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm b/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm >index 903635af525a8b1f447060314307d1083d3635ad..6ba972063e60b68d3dbcfa49f8d9c9d95fcc3f5b 100644 >--- a/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm >+++ b/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm >@@ -422,37 +422,36 @@ macro checkSwitchToJITForLoop() > end) > end > >-macro uncage(basePtr, mask, ptr, scratchOrLength) >+macro cage(basePtr, mask, ptr, scratch) > if GIGACAGE_ENABLED and not (C_LOOP or C_LOOP_WIN) >- loadp basePtr, scratchOrLength >- btpz scratchOrLength, .done >+ loadp basePtr, scratch >+ btpz scratch, .done > andp mask, ptr >- addp scratchOrLength, ptr >+ addp scratch, ptr > .done: > end > end > >-macro loadCagedPrimitive(source, dest, scratchOrLength) >- loadp source, dest >+macro cagedPrimitive(ptr, length, scratch, scratch2) > if ARM64E >- const result = t7 >- untagArrayPtr scratchOrLength, dest >- move dest, result >+ const source = scratch2 >+ move ptr, scratch2 > else >- const result = dest >+ const source = ptr > end > if GIGACAGE_ENABLED >- uncage(_g_gigacageBasePtrs + Gigacage::BasePtrs::primitive, constexpr Gigacage::primitiveGigacageMask, result, scratchOrLength) >+ cage(_g_gigacageBasePtrs + Gigacage::BasePtrs::primitive, constexpr Gigacage::primitiveGigacageMask, source, scratch) > if ARM64E > const numberOfPACBits = constexpr MacroAssembler::numberOfPACBits >- bfiq result, 0, 64 - numberOfPACBits, dest >+ bfiq scratch2, 0, 64 - numberOfPACBits, ptr >+ untagArrayPtr length, ptr > end > end > end > > macro loadCagedJSValue(source, dest, scratchOrLength) > loadp source, dest >- uncage(_g_gigacageBasePtrs + Gigacage::BasePtrs::jsValue, constexpr Gigacage::jsValueGigacageMask, dest, scratchOrLength) >+ cage(_g_gigacageBasePtrs + Gigacage::BasePtrs::jsValue, constexpr Gigacage::jsValueGigacageMask, dest, scratchOrLength) > end > > macro loadVariable(get, fieldName, valueReg) >@@ -1522,15 +1521,17 @@ llintOpWithMetadata(op_get_by_val, OpGetByVal, macro (size, get, dispatch, metad > # Sweet, now we know that we have a typed array. Do some basic things now. > > if ARM64E >- const scratchOrLength = t6 >- loadi JSArrayBufferView::m_length[t0], scratchOrLength >- biaeq t1, scratchOrLength, .opGetByValSlow >+ const length = t6 >+ const scratch = t7 >+ loadi JSArrayBufferView::m_length[t0], length >+ biaeq t1, length, .opGetByValSlow > else >- const scratchOrLength = t0 >+ # length and scratch are intentionally undefined on this branch because they are not used on other platforms. > biaeq t1, JSArrayBufferView::m_length[t0], .opGetByValSlow > end > >- loadCagedPrimitive(JSArrayBufferView::m_vector[t0], t3, scratchOrLength) >+ loadp JSArrayBufferView::m_vector[t0], t3 >+ cagedPrimitive(t3, length, t0, scratch) > > # Now bisect through the various types: > # Int8ArrayType, >diff --git a/Source/WTF/wtf/CagedPtr.h b/Source/WTF/wtf/CagedPtr.h >index ca473c4baaa9955bde8556ccd5eb1b9e103caadc..5210e7157ae0a8d840fc7883e11a7125ae7ce6a4 100644 >--- a/Source/WTF/wtf/CagedPtr.h >+++ b/Source/WTF/wtf/CagedPtr.h >@@ -39,6 +39,8 @@ template<Gigacage::Kind passedKind, typename T, bool shouldTag = false, typename > class CagedPtr { > public: > static constexpr Gigacage::Kind kind = passedKind; >+ static constexpr unsigned numberOfPACBits = 25; >+ static constexpr uintptr_t nonPACBitsMask = (1ull << ((sizeof(T*) * CHAR_BIT) - numberOfPACBits)) - 1; > > CagedPtr() : CagedPtr(nullptr) { } > CagedPtr(std::nullptr_t) >@@ -49,20 +51,23 @@ public: > : m_ptr(shouldTag ? tagArrayPtr(ptr, size) : ptr) > { } > >- > T* get(unsigned size) const > { > ASSERT(m_ptr); > T* ptr = PtrTraits::unwrap(m_ptr); >- T* untaggedPtr = shouldTag ? untagArrayPtr(ptr, size) : ptr; >- return mergePointers(untaggedPtr, Gigacage::caged(kind, ptr)); >+ T* cagedPtr = Gigacage::caged(kind, ptr); >+ T* untaggedPtr = shouldTag ? untagArrayPtr(mergePointers(ptr, cagedPtr), size) : cagedPtr; >+ return untaggedPtr; > } > > T* getMayBeNull(unsigned size) const > { > T* ptr = PtrTraits::unwrap(m_ptr); >- T* untaggedPtr = shouldTag ? untagArrayPtr(ptr, size) : ptr; >- return mergePointers(untaggedPtr, Gigacage::cagedMayBeNull(kind, ptr)); >+ if (!removeArrayPtrTag(ptr)) >+ return nullptr; >+ T* cagedPtr = Gigacage::caged(kind, ptr); >+ T* untaggedPtr = shouldTag ? untagArrayPtr(mergePointers(ptr, cagedPtr), size) : cagedPtr; >+ return untaggedPtr; > } > > T* getUnsafe() const >@@ -124,11 +129,14 @@ public: > } > > protected: >- static inline T* mergePointers(const T* untaggedPtr, const T* uncagedPtr) >+ static inline T* mergePointers(T* sourcePtr, T* cagedPtr) > { >- constexpr unsigned numberOfPACBits = 25; >- constexpr uintptr_t mask = (1ull << ((sizeof(T*) * CHAR_BIT) - numberOfPACBits)) - 1; >- return reinterpret_cast<T*>((reinterpret_cast<uintptr_t>(untaggedPtr) & ~mask) | (reinterpret_cast<uintptr_t>(uncagedPtr) & mask)); >+#if CPU(ARM64E) >+ return reinterpret_cast<T*>((reinterpret_cast<uintptr_t>(sourcePtr) & ~nonPACBitsMask) | (reinterpret_cast<uintptr_t>(cagedPtr) & nonPACBitsMask)); >+#else >+ UNUSED_PARAM(sourcePtr); >+ return cagedPtr; >+#endif > } > > typename PtrTraits::StorageType m_ptr; >diff --git a/Source/bmalloc/bmalloc/ProcessCheck.mm b/Source/bmalloc/bmalloc/ProcessCheck.mm >index 3bb63f3525429898b0994cc851793b5195f52b02..b872c59f4fe44cc0b376d40aa3f9a77e131e6ca1 100644 >--- a/Source/bmalloc/bmalloc/ProcessCheck.mm >+++ b/Source/bmalloc/bmalloc/ProcessCheck.mm >@@ -66,7 +66,9 @@ bool shouldProcessUnconditionallyUseBmalloc() > result = contains(@"com.apple.WebKit") || contains(@"safari"); > } else { > NSString *processName = [[NSProcessInfo processInfo] processName]; >- result = [processName isEqualToString:@"jsc"] || [processName isEqualToString:@"wasm"]; >+ result = [processName isEqualToString:@"jsc"] >+ || [processName isEqualToString:@"wasm"] >+ || [processName hasPrefix:@"test"]; > } > }); >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=373409">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 199372
:
373254
|
373289
| 373409