From b2df815a36cd260168771fc94946ce515e23d3a4 Mon Sep 17 00:00:00 2001 From: "keith_miller@apple.com" Date: Tue, 2 Jul 2019 07:00:14 +0000 Subject: [PATCH] PACCage should first cage leaving PAC bits intact then authenticate https://bugs.webkit.org/show_bug.cgi?id=199372 Reviewed by Saam Barati. Source/bmalloc: * bmalloc/ProcessCheck.mm: (bmalloc::shouldProcessUnconditionallyUseBmalloc): Source/JavaScriptCore: 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: Source/WTF: * wtf/CagedPtr.h: (WTF::CagedPtr::get const): (WTF::CagedPtr::getMayBeNull const): (WTF::CagedPtr::mergePointers): git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247041 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- Source/JavaScriptCore/ChangeLog | 60 ++++++++++++++++++++++ .../assembler/MacroAssemblerARM64E.h | 1 + Source/JavaScriptCore/assembler/testmasm.cpp | 22 +++++--- Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp | 23 ++++----- Source/JavaScriptCore/jit/AssemblyHelpers.h | 57 ++++++++++---------- .../JavaScriptCore/llint/LowLevelInterpreter64.asm | 37 ++++++------- Source/WTF/ChangeLog | 12 +++++ Source/WTF/wtf/CagedPtr.h | 20 +++++--- Source/bmalloc/ChangeLog | 10 ++++ Source/bmalloc/bmalloc/ProcessCheck.mm | 4 +- 10 files changed, 170 insertions(+), 76 deletions(-) diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog index a249548..1d6af2b 100644 --- a/Source/JavaScriptCore/ChangeLog +++ b/Source/JavaScriptCore/ChangeLog @@ -1,3 +1,63 @@ +2019-07-02 Keith Miller + + 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-01 Justin Michaud [Wasm-References] Disable references by default diff --git a/Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h b/Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h index fd7eec8..9c6e775 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 1050b39..3cbaed8 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(reinterpret_cast(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(cage, taggedPtr, 2), ptr); + CHECK_EQ(invoke(cage, taggedNotCagedPtr, 1), untagArrayPtr(taggedPtr, 2)); } else CHECK_EQ(invoke(cage, taggedPtr, 2), ptr); @@ -1042,16 +1049,17 @@ static void testCagePreservesPACFailureBit() jit.ret(); }); + CHECK_EQ(invoke(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(cageWithoutAuthentication, untagArrayPtr(taggedPtr, 2)), ptr); - } else - CHECK_EQ(invoke(cageWithoutAuthentication, untagArrayPtr(taggedPtr, 2)), ptr); - - CHECK_EQ(untagArrayPtr(taggedPtr, 1), ptr); - CHECK_EQ(invoke(cageWithoutAuthentication, untagArrayPtr(taggedPtr, 1)), ptr); + CHECK_NOT_EQ(invoke(cageWithoutAuthentication, taggedNotCagedPtr), taggedNotCagedPtr); + CHECK_NOT_EQ(untagArrayPtr(invoke(cageWithoutAuthentication, taggedNotCagedPtr), 1), notCagedPtr); + CHECK_NOT_EQ(invoke(cageWithoutAuthentication, taggedNotCagedPtr), taggedPtr); + CHECK_NOT_EQ(untagArrayPtr(invoke(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 55a2897..1af7414 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 a77d43a..3eead2d 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 903635a..6ba9720 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/ChangeLog b/Source/WTF/ChangeLog index c2f305e..2534920 100644 --- a/Source/WTF/ChangeLog +++ b/Source/WTF/ChangeLog @@ -1,3 +1,15 @@ +2019-07-02 Keith Miller + + 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-01 Philippe Normand [GStreamer] Cannot play Bert's Bytes radio stream from http://radio.dos.nl/ diff --git a/Source/WTF/wtf/CagedPtr.h b/Source/WTF/wtf/CagedPtr.h index ca473c4..b438301 100644 --- a/Source/WTF/wtf/CagedPtr.h +++ b/Source/WTF/wtf/CagedPtr.h @@ -49,20 +49,21 @@ 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)); + T* cagedPtr = Gigacage::cagedMayBeNull(kind, ptr); + T* untaggedPtr = shouldTag ? untagArrayPtr(mergePointers(ptr, cagedPtr), size) : cagedPtr; + return untaggedPtr; } T* getUnsafe() const @@ -124,11 +125,16 @@ public: } protected: - static inline T* mergePointers(const T* untaggedPtr, const T* uncagedPtr) + static inline T* mergePointers(T* sourcePtr, T* cagedPtr) { +#if CPU(ARM64E) constexpr unsigned numberOfPACBits = 25; constexpr uintptr_t mask = (1ull << ((sizeof(T*) * CHAR_BIT) - numberOfPACBits)) - 1; - return reinterpret_cast((reinterpret_cast(untaggedPtr) & ~mask) | (reinterpret_cast(uncagedPtr) & mask)); + return reinterpret_cast((reinterpret_cast(sourcePtr) & ~mask) | (reinterpret_cast(cagedPtr) & mask)); +#else + UNUSED_PARAM(sourcePtr); + return cagedPtr; +#endif } typename PtrTraits::StorageType m_ptr; diff --git a/Source/bmalloc/ChangeLog b/Source/bmalloc/ChangeLog index 64ac685..97b57cb 100644 --- a/Source/bmalloc/ChangeLog +++ b/Source/bmalloc/ChangeLog @@ -1,3 +1,13 @@ +2019-07-02 Keith Miller + + 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-06-27 Beth Dakin Upstream use of MACCATALYST diff --git a/Source/bmalloc/bmalloc/ProcessCheck.mm b/Source/bmalloc/bmalloc/ProcessCheck.mm index 3bb63f3..b872c59 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"]; } }); -- 1.8.3.1