[sandbox] Store external pointer table indices shifted to the left

This guarantees that they are smaller than the maximum external pointer
table index when shifted to the right on load.

Bug: v8:10391
Change-Id: I601f37fbb9640ee4b5215958afcc474c5e0eb9af
Cq-Include-Trybots: luci.v8.try:v8_linux64_heap_sandbox_dbg_ng,v8_linux_arm64_sim_heap_sandbox_dbg_ng
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3359631
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Maya Lekova <mslekova@chromium.org>
Commit-Queue: Samuel Groß <saelo@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78873}

include/v8-context.h

@@ -368,17 +368,17 @@ Local<Value> Context::GetEmbedderData(int index) {
 }
 
 void* Context::GetAlignedPointerFromEmbedderData(int index) {
-#ifndef V8_ENABLE_CHECKS
-  using A = internal::Address;
   using I = internal::Internals;
+  static_assert(I::kEmbedderDataSlotSize == internal::kApiSystemPointerSize,
+                "Enable fast path with sandboxed external pointers enabled "
+                "once embedder data slots are 32 bits large");
+#if !defined(V8_ENABLE_CHECKS) && !defined(V8_SANDBOXED_EXTERNAL_POINTERS)
+  using A = internal::Address;
   A ctx = *reinterpret_cast<const A*>(this);
   A embedder_data =
       I::ReadTaggedPointerField(ctx, I::kNativeContextEmbedderDataOffset);
   int value_offset =
       I::kEmbedderDataArrayHeaderSize + (I::kEmbedderDataSlotSize * index);
-#ifdef V8_SANDBOXED_EXTERNAL_POINTERS
-  value_offset += I::kEmbedderDataSlotRawPayloadOffset;
-#endif
   internal::Isolate* isolate = I::GetIsolateForSandbox(ctx);
   return reinterpret_cast<void*>(
       I::ReadExternalPointerField(isolate, embedder_data, value_offset,

include/v8-internal.h

@@ -256,6 +256,26 @@ constexpr bool kAllowBackingStoresOutsideSandbox = false;
 constexpr bool kAllowBackingStoresOutsideSandbox = true;
 #endif  // V8_SANDBOXED_POINTERS
 
+// The size of the virtual memory reservation for an external pointer table.
+// This determines the maximum number of entries in a table. Using a maximum
+// size allows omitting bounds checks on table accesses if the indices are
+// guaranteed (e.g. through shifting) to be below the maximum index. This
+// value must be a power of two.
+static const size_t kExternalPointerTableReservationSize = 128 * MB;
+
+// The maximum number of entries in an external pointer table.
+static const size_t kMaxSandboxedExternalPointers =
+    kExternalPointerTableReservationSize / kApiSystemPointerSize;
+
+// The external pointer table indices stored in HeapObjects as external
+// pointers are shifted to the left by this amount to guarantee that they are
+// smaller than the maximum table size.
+static const uint32_t kExternalPointerIndexShift = 8;
+static_assert((1 << (32 - kExternalPointerIndexShift)) ==
+                  kMaxSandboxedExternalPointers,
+              "kExternalPointerTableReservationSize and "
+              "kExternalPointerIndexShift don't match");
+
 #endif  // V8_SANDBOX_IS_AVAILABLE
 
 // If sandboxed external pointers are enabled, these tag values will be ORed
@@ -288,17 +308,6 @@ enum ExternalPointerTag : uint64_t {
 
 constexpr uint64_t kExternalPointerTagMask = 0xffff000000000000;
 
-// The size of the virtual memory reservation for an external pointer table.
-// This determines the maximum number of entries in a table. Using a maximum
-// size allows omitting bounds checks on table accesses if the indices are
-// guaranteed (e.g. through shifting) to be below the maximum index. This
-// value must be a power of two.
-static const size_t kExternalPointerTableReservationSize = 128 * MB;
-
-// The maximum number of entries in an external pointer table.
-static const size_t kMaxSandboxedExternalPointers =
-    kExternalPointerTableReservationSize / kApiSystemPointerSize;
-
 // Converts encoded external pointer to address.
 V8_EXPORT Address DecodeExternalPointerImpl(const Isolate* isolate,
                                             ExternalPointer_t pointer,

include/v8-object.h

@@ -735,18 +735,18 @@ Local<Value> Object::GetInternalField(int index) {
 }
 
 void* Object::GetAlignedPointerFromInternalField(int index) {
-#ifndef V8_ENABLE_CHECKS
-  using A = internal::Address;
   using I = internal::Internals;
+  static_assert(I::kEmbedderDataSlotSize == internal::kApiSystemPointerSize,
+                "Enable fast path with sandboxed external pointers enabled "
+                "once embedder data slots are 32 bits large");
+#if !defined(V8_ENABLE_CHECKS) && !defined(V8_SANDBOXED_EXTERNAL_POINTERS)
+  using A = internal::Address;
   A obj = *reinterpret_cast<A*>(this);
   // Fast path: If the object is a plain JSObject, which is the common case, we
   // know where to find the internal fields and can return the value directly.
   auto instance_type = I::GetInstanceType(obj);
   if (v8::internal::CanHaveInternalField(instance_type)) {
     int offset = I::kJSObjectHeaderSize + (I::kEmbedderDataSlotSize * index);
-#ifdef V8_SANDBOXED_EXTERNAL_POINTERS
-    offset += I::kEmbedderDataSlotRawPayloadOffset;
-#endif
     internal::Isolate* isolate = I::GetIsolateForSandbox(obj);
     A value = I::ReadExternalPointerField(
         isolate, obj, offset, internal::kEmbedderDataSlotPayloadTag);

src/codegen/arm64/macro-assembler-arm64.cc

@@ -3109,6 +3109,7 @@ void TurboAssembler::LoadExternalPointerField(Register destination,
   DCHECK(!AreAliased(destination, isolate_root));
   ASM_CODE_COMMENT(this);
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
+  DCHECK_NE(kExternalPointerNullTag, tag);
   UseScratchRegisterScope temps(this);
   Register external_table = temps.AcquireX();
   if (isolate_root == no_reg) {
@@ -3120,11 +3121,13 @@ void TurboAssembler::LoadExternalPointerField(Register destination,
                  IsolateData::external_pointer_table_offset() +
                      Internals::kExternalPointerTableBufferOffset));
   Ldr(destination.W(), field_operand);
-  Ldr(destination,
+  // MemOperand doesn't support LSR currently (only LSL), so here we do the
-      MemOperand(external_table, destination, LSL, kSystemPointerSizeLog2));
+  // offset computation separately first.
-  if (tag != 0) {
+  STATIC_ASSERT(kExternalPointerIndexShift > kSystemPointerSizeLog2);
-    And(destination, destination, Immediate(~tag));
+  int shift_amount = kExternalPointerIndexShift - kSystemPointerSizeLog2;
-  }
+  Mov(destination, Operand(destination, LSR, shift_amount));
+  Ldr(destination, MemOperand(external_table, destination));
+  And(destination, destination, Immediate(~tag));
 #else
   Ldr(destination, field_operand);
 #endif  // V8_SANDBOXED_EXTERNAL_POINTERS

src/codegen/code-stub-assembler.cc

@@ -1584,17 +1584,22 @@ TNode<RawPtrT> CodeStubAssembler::EmptyBackingStoreBufferConstant() {
 #endif  // V8_SANDBOXED_POINTERS
 }
 
-TNode<ExternalPointerT> CodeStubAssembler::ChangeUint32ToExternalPointer(
+#ifdef V8_SANDBOXED_EXTERNAL_POINTERS
-    TNode<Uint32T> value) {
+TNode<ExternalPointerT> CodeStubAssembler::ChangeIndexToExternalPointer(
+    TNode<Uint32T> index) {
   DCHECK_EQ(kExternalPointerSize, kUInt32Size);
-  return ReinterpretCast<ExternalPointerT>(value);
+  TNode<Uint32T> shifted_index =
+      Word32Shl(index, Uint32Constant(kExternalPointerIndexShift));
+  return ReinterpretCast<ExternalPointerT>(shifted_index);
 }
 
-TNode<Uint32T> CodeStubAssembler::ChangeExternalPointerToUint32(
+TNode<Uint32T> CodeStubAssembler::ChangeExternalPointerToIndex(
-    TNode<ExternalPointerT> value) {
+    TNode<ExternalPointerT> external_pointer) {
   DCHECK_EQ(kExternalPointerSize, kUInt32Size);
-  return ReinterpretCast<Uint32T>(value);
+  TNode<Uint32T> shifted_index = ReinterpretCast<Uint32T>(external_pointer);
+  return Word32Shr(shifted_index, Uint32Constant(kExternalPointerIndexShift));
 }
+#endif  // V8_SANDBOXED_EXTERNAL_POINTERS
 
 void CodeStubAssembler::InitializeExternalPointerField(TNode<HeapObject> object,
                                                        TNode<IntPtrT> offset) {
@@ -1656,9 +1661,8 @@ void CodeStubAssembler::InitializeExternalPointerField(TNode<HeapObject> object,
   // real value). TODO(saelo) initialize the entry with zero here and switch
   // callers to a version that initializes the entry with a given pointer.
 
-  TNode<ExternalPointerT> encoded =
+  TNode<ExternalPointerT> pointer = ChangeIndexToExternalPointer(index.value());
-      ChangeUint32ToExternalPointer(index.value());
+  StoreObjectFieldNoWriteBarrier<ExternalPointerT>(object, offset, pointer);
-  StoreObjectFieldNoWriteBarrier<ExternalPointerT>(object, offset, encoded);
 #endif
 }
 
@@ -1674,7 +1678,9 @@ TNode<RawPtrT> CodeStubAssembler::LoadExternalPointerFromObject(
 
   TNode<ExternalPointerT> encoded =
       LoadObjectField<ExternalPointerT>(object, offset);
-  TNode<Word32T> index = ChangeExternalPointerToUint32(encoded);
+  TNode<Uint32T> index = ChangeExternalPointerToIndex(encoded);
+  // TODO(v8:10391): consider updating ElementOffsetFromIndex to generate code
+  // that does one shift right instead of two shifts (right and then left).
   TNode<IntPtrT> table_offset = ElementOffsetFromIndex(
       ChangeUint32ToWord(index), SYSTEM_POINTER_ELEMENTS, 0);
 
@@ -1701,8 +1707,9 @@ void CodeStubAssembler::StoreExternalPointerToObject(
 
   TNode<ExternalPointerT> encoded =
       LoadObjectField<ExternalPointerT>(object, offset);
-  TNode<Word32T> index = ChangeExternalPointerToUint32(encoded);
+  TNode<Uint32T> index = ChangeExternalPointerToIndex(encoded);
-  // TODO(v8:10391, saelo): bounds check if table is not caged
+  // TODO(v8:10391): consider updating ElementOffsetFromIndex to generate code
+  // that does one shift right instead of two shifts (right and then left).
   TNode<IntPtrT> table_offset = ElementOffsetFromIndex(
       ChangeUint32ToWord(index), SYSTEM_POINTER_ELEMENTS, 0);
 

src/codegen/code-stub-assembler.h

@@ -1093,8 +1093,10 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   // ExternalPointerT-related functionality.
   //
 
-  TNode<ExternalPointerT> ChangeUint32ToExternalPointer(TNode<Uint32T> value);
+#ifdef V8_SANDBOXED_EXTERNAL_POINTERS
-  TNode<Uint32T> ChangeExternalPointerToUint32(TNode<ExternalPointerT> value);
+  TNode<ExternalPointerT> ChangeIndexToExternalPointer(TNode<Uint32T> index);
+  TNode<Uint32T> ChangeExternalPointerToIndex(TNode<ExternalPointerT> pointer);
+#endif  // V8_SANDBOXED_EXTERNAL_POINTERS
 
   // Initialize an external pointer field in an object.
   void InitializeExternalPointerField(TNode<HeapObject> object, int offset) {

src/codegen/x64/macro-assembler-x64.cc

@@ -418,6 +418,7 @@ void TurboAssembler::LoadExternalPointerField(
     Register scratch, IsolateRootLocation isolateRootLocation) {
   DCHECK(!AreAliased(destination, scratch));
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
+  DCHECK_NE(kExternalPointerNullTag, tag);
   DCHECK(!field_operand.AddressUsesRegister(scratch));
   if (isolateRootLocation == IsolateRootLocation::kInRootRegister) {
     DCHECK(root_array_available_);
@@ -431,11 +432,10 @@ void TurboAssembler::LoadExternalPointerField(
                               Internals::kExternalPointerTableBufferOffset));
   }
   movl(destination, field_operand);
+  shrq(destination, Immediate(kExternalPointerIndexShift));
   movq(destination, Operand(scratch, destination, times_8, 0));
-  if (tag != 0) {
+  movq(scratch, Immediate64(~tag));
-    movq(scratch, Immediate64(~tag));
+  andq(destination, scratch);
-    andq(destination, scratch);
-  }
 #else
   movq(destination, field_operand);
 #endif  // V8_SANDBOXED_EXTERNAL_POINTERS

src/compiler/memory-lowering.cc

@@ -412,6 +412,7 @@ Node* MemoryLowering::DecodeExternalPointer(
   DCHECK(V8_SANDBOXED_EXTERNAL_POINTERS_BOOL);
   DCHECK(node->opcode() == IrOpcode::kLoad);
   DCHECK_EQ(kExternalPointerSize, kUInt32Size);
+  DCHECK_NE(kExternalPointerNullTag, external_pointer_tag);
   Node* effect = NodeProperties::GetEffectInput(node);
   Node* control = NodeProperties::GetControlInput(node);
   __ InitializeEffectControl(effect, control);
@@ -419,7 +420,11 @@ Node* MemoryLowering::DecodeExternalPointer(
   // Clone the load node and put it here.
   // TODO(turbofan): consider adding GraphAssembler::Clone() suitable for
   // cloning nodes from arbitrary locaions in effect/control chains.
-  Node* index = __ AddNode(graph()->CloneNode(node));
+  STATIC_ASSERT(kExternalPointerIndexShift > kSystemPointerSizeLog2);
+  Node* shifted_index = __ AddNode(graph()->CloneNode(node));
+  Node* shift_amount =
+      __ Int32Constant(kExternalPointerIndexShift - kSystemPointerSizeLog2);
+  Node* offset = __ Word32Shr(shifted_index, shift_amount);
 
   // Uncomment this to generate a breakpoint for debugging purposes.
   // __ DebugBreak();
@@ -436,13 +441,10 @@ Node* MemoryLowering::DecodeExternalPointer(
       ExternalReference::external_pointer_table_address(isolate()));
   Node* table = __ Load(MachineType::Pointer(), table_address,
                         Internals::kExternalPointerTableBufferOffset);
-  Node* offset = __ Int32Mul(index, __ Int32Constant(sizeof(Address)));
   Node* decoded_ptr =
       __ Load(MachineType::Pointer(), table, __ ChangeUint32ToUint64(offset));
-  if (external_pointer_tag != 0) {
+  Node* tag = __ IntPtrConstant(~external_pointer_tag);
-    Node* tag = __ IntPtrConstant(~external_pointer_tag);
+  decoded_ptr = __ WordAnd(decoded_ptr, tag);
-    decoded_ptr = __ WordAnd(decoded_ptr, tag);
-  }
   return decoded_ptr;
 #else
   return node;

src/compiler/wasm-compiler.cc

@@ -3224,13 +3224,15 @@ Node* WasmGraphBuilder::BuildLoadExternalPointerFromObject(
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
   Node* external_pointer = gasm_->LoadFromObject(
       MachineType::Uint32(), object, wasm::ObjectAccess::ToTagged(offset));
+  STATIC_ASSERT(kExternalPointerIndexShift > kSystemPointerSizeLog2);
+  Node* shift_amount =
+      gasm_->Int32Constant(kExternalPointerIndexShift - kSystemPointerSizeLog2);
+  Node* scaled_index = gasm_->Word32Shr(external_pointer, shift_amount);
   Node* isolate_root = BuildLoadIsolateRoot();
   Node* table =
       gasm_->LoadFromObject(MachineType::Pointer(), isolate_root,
                             IsolateData::external_pointer_table_offset() +
                                 Internals::kExternalPointerTableBufferOffset);
-  Node* scaled_index = gasm_->Int32Mul(
-      external_pointer, gasm_->Int32Constant(kSystemPointerSize));
   Node* decoded_ptr = gasm_->Load(MachineType::Pointer(), table, scaled_index);
   return gasm_->WordAnd(decoded_ptr, gasm_->IntPtrConstant(~tag));
 #else

src/heap/marking-visitor.h

@@ -198,7 +198,8 @@ class MarkingVisitorBase : public HeapVisitor<int, ConcreteVisitor> {
   V8_INLINE void VisitExternalPointer(HeapObject host,
                                       ExternalPointer_t ptr) final {
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
-    external_pointer_table_->Mark(static_cast<uint32_t>(ptr));
+    uint32_t index = ptr >> kExternalPointerIndexShift;
+    external_pointer_table_->Mark(index);
 #endif  // V8_SANDBOXED_EXTERNAL_POINTERS
   }
 

src/logging/counters-definitions.h

@@ -111,10 +111,17 @@ namespace internal {
      3)                                                                        \
   /* number of times a cache event is triggered for a wasm module */           \
   HR(wasm_cache_count, V8.WasmCacheCount, 0, 100, 101)                         \
-  /* Number of in-use external pointers in the external pointer table */       \
+  SANDBOXED_HISTOGRAM_LIST(HR)
-  /* Counted after sweeping the table at the end of mark-compact GC */         \
+
-  HR(sandboxed_external_pointers_count, V8.SandboxedExternalPointersCount, 0,  \
+#ifdef V8_SANDBOX_IS_AVAILABLE
+#define SANDBOXED_HISTOGRAM_LIST(HR)                                          \
+  /* Number of in-use external pointers in the external pointer table */      \
+  /* Counted after sweeping the table at the end of mark-compact GC */        \
+  HR(sandboxed_external_pointers_count, V8.SandboxedExternalPointersCount, 0, \
      kMaxSandboxedExternalPointers, 101)
+#else
+#define SANDBOXED_HISTOGRAM_LIST(HR)
+#endif  // V8_SANDBOX_IS_AVAILABLE
 
 #define NESTED_TIMED_HISTOGRAM_LIST(HT)                                       \
   /* Timer histograms, not thread safe: HT(name, caption, max, unit) */       \

src/sandbox/external-pointer-inl.h

@@ -18,7 +18,8 @@ V8_INLINE Address DecodeExternalPointer(const Isolate* isolate,
                                         ExternalPointerTag tag) {
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
   STATIC_ASSERT(kExternalPointerSize == kInt32Size);
-  return isolate->external_pointer_table().Get(encoded_pointer, tag);
+  uint32_t index = encoded_pointer >> kExternalPointerIndexShift;
+  return isolate->external_pointer_table().Get(index, tag);
 #else
   STATIC_ASSERT(kExternalPointerSize == kSystemPointerSize);
   return encoded_pointer;
@@ -35,6 +36,7 @@ V8_INLINE void InitExternalPointerField(Address field_address, Isolate* isolate,
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
   ExternalPointer_t index = isolate->external_pointer_table().Allocate();
   isolate->external_pointer_table().Set(index, value, tag);
+  index <<= kExternalPointerIndexShift;
   base::Memory<ExternalPointer_t>(field_address) = index;
 #else
   // Pointer compression causes types larger than kTaggedSize to be unaligned.
@@ -72,6 +74,7 @@ V8_INLINE void WriteExternalPointerField(Address field_address,
                                          ExternalPointerTag tag) {
 #ifdef V8_SANDBOXED_EXTERNAL_POINTERS
   ExternalPointer_t index = base::Memory<ExternalPointer_t>(field_address);
+  index >>= kExternalPointerIndexShift;
   isolate->external_pointer_table().Set(index, value, tag);
 #else
   // Pointer compression causes types larger than kTaggedSize to be unaligned.