PACCage should first cage leaving PAC bits intact then authenticate
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Jul 2019 07:00:14 +0000 (07:00 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Jul 2019 07:00:14 +0000 (07:00 +0000)
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
Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h
Source/JavaScriptCore/assembler/testmasm.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/jit/AssemblyHelpers.h
Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
Source/WTF/ChangeLog
Source/WTF/wtf/CagedPtr.h
Source/bmalloc/ChangeLog
Source/bmalloc/bmalloc/ProcessCheck.mm

index a249548..1d6af2b 100644 (file)
@@ -1,3 +1,63 @@
+2019-07-02  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-01  Justin Michaud  <justin_michaud@apple.com>
 
         [Wasm-References] Disable references by default
index fd7eec8..9c6e775 100644 (file)
@@ -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()
     {
index 1050b39..3cbaed8 100644 (file)
@@ -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 {                          \
index 55a2897..1af7414 100644 (file)
@@ -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;
     }
     
index a77d43a..3eead2d 100644 (file)
@@ -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)
index 903635a..6ba9720 100644 (file)
@@ -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,
index c2f305e..2534920 100644 (file)
@@ -1,3 +1,15 @@
+2019-07-02  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-01  Philippe Normand  <pnormand@igalia.com>
 
         [GStreamer] Cannot play Bert's Bytes radio stream from http://radio.dos.nl/
index ca473c4..b438301 100644 (file)
@@ -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<T*>((reinterpret_cast<uintptr_t>(untaggedPtr) & ~mask) | (reinterpret_cast<uintptr_t>(uncagedPtr) & mask));
+        return reinterpret_cast<T*>((reinterpret_cast<uintptr_t>(sourcePtr) & ~mask) | (reinterpret_cast<uintptr_t>(cagedPtr) & mask));
+#else
+        UNUSED_PARAM(sourcePtr);
+        return cagedPtr;
+#endif
     }
 
     typename PtrTraits::StorageType m_ptr;
index 64ac685..97b57cb 100644 (file)
@@ -1,3 +1,13 @@
+2019-07-02  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-06-27  Beth Dakin  <bdakin@apple.com>
 
         Upstream use of MACCATALYST
index 3bb63f3..b872c59 100644 (file)
@@ -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"];
         }
     });