WebKit Bugzilla
Attachment 373254 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-20190701134655.patch (text/plain), 9.45 KB, created by
Keith Miller
on 2019-07-01 13:46:56 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Keith Miller
Created:
2019-07-01 13:46:56 PDT
Size:
9.45 KB
patch
obsolete
>Subversion Revision: 246837 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index c974eedc160131db94f427c18dac7291ff3c28e2..948f53b49f6e0b88cb1f546344346560b7e0e933 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,23 @@ >+2019-07-01 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 NOBODY (OOPS!). >+ >+ 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. >+ >+ * assembler/MacroAssemblerARM64E.h: >+ * assembler/testmasm.cpp: >+ (JSC::testCagePreservesPACFailureBit): >+ * ftl/FTLLowerDFGToB3.cpp: >+ (JSC::FTL::DFG::LowerDFGToB3::caged): >+ * jit/AssemblyHelpers.h: >+ (JSC::AssemblyHelpers::cageConditionally): >+ > 2019-06-26 Keith Miller <keith_miller@apple.com> > > remove unneeded didBecomePrototype() calls >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 c89010a1554ac72c56c61a303670b2079ef1f75f..3306d53a1cdb4b5d74c9918b7a40bbec477b65f4 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)
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=373254">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