Changeset 247041 in webkit

Timestamp:

Jul 2, 2019 12:00:14 AM (5 years ago)

Author:

keith_miller@apple.com

Message:

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):

Location:

trunk/Source

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/assembler/MacroAssemblerARM64E.h (modified) (1 diff)
• JavaScriptCore/assembler/testmasm.cpp (modified) (3 diffs)
• JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (modified) (4 diffs)
• JavaScriptCore/jit/AssemblyHelpers.h (modified) (1 diff)
• JavaScriptCore/llint/LowLevelInterpreter64.asm (modified) (3 diffs)
• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/CagedPtr.h (modified) (3 diffs)
• bmalloc/ChangeLog (modified) (1 diff)
• bmalloc/bmalloc/ProcessCheck.mm (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h

@@ -41 +41 @@
 public:
     static constexpr unsigned numberOfPACBits = 25;
+    static constexpr uintptr_t nonPACBitsMask = (1ull << (64 - numberOfPACBits)) - 1;
 
     ALWAYS_INLINE void tagReturnAddress()

trunk/Source/JavaScriptCore/assembler/testmasm.cpp

@@ -1016 +1016 @@
 static void testCagePreservesPACFailureBit()
 {
+#if GIGACAGE_ENABLED
+    ASSERT(!Gigacage::isDisablingPrimitiveGigacageDisabled());
     auto cage = compile([] (CCallHelpers& jit) {
         jit.emitFunctionPrologue();
@@ -1026 +1028 @@
     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);
@@ -1043 +1050 @@
     });
 
+    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
 }
 

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -14178 +14178 @@
     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;
@@ -14207 +14196 @@
 
 #if CPU(ARM64E)
-        {
+        if (kind == Gigacage::Primitive) {
             PatchpointValue* merge = m_out.patchpoint(pointerType());
             merge->append(result, B3::ValueRep(B3::ValueRep::SomeLateRegister));
@@ -14215 +14204 @@
                 jit.bitFieldInsert64(params[1].gpr(), 0, 64 - MacroAssembler::numberOfPACBits, params[0].gpr());
             });
-            result = merge;
-        }
-#endif
+
+            LValue size = m_out.load32(base, m_heaps.JSArrayBufferView_length);
+            result = untagArrayPtr(merge, size);
+        }
+#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
@@ -14231 +14223 @@
         return m_out.opaque(result);
 #endif
+
+        UNUSED_PARAM(kind);
+        UNUSED_PARAM(base);
         return ptr;
     }

trunk/Source/JavaScriptCore/jit/AssemblyHelpers.h

@@ -1587 +1587 @@
     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
-
     }
 

trunk/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm

@@ -423 +423 @@
 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
@@ -453 +452 @@
 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
 
@@ -1523 +1522 @@
 
     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:

trunk/Source/WTF/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WTF/wtf/CagedPtr.h

@@ -50 +50 @@
     { }
 
-
     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;
     }
 
@@ -62 +62 @@
     {
         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;
     }
 
@@ -125 +126 @@
     
 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
     }
 

trunk/Source/bmalloc/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/bmalloc/bmalloc/ProcessCheck.mm

@@ -67 +67 @@
         } else {
             NSString *processName = [[NSProcessInfo processInfo] processName];
-            result = [processName isEqualToString:@"jsc"] || [processName isEqualToString:@"wasm"];
+            result = [processName isEqualToString:@"jsc"]
+                || [processName isEqualToString:@"wasm"]
+                || [processName hasPrefix:@"test"];
         }
     });