From 951cdb3606567f371d390125f40736d985f62e89 Mon Sep 17 00:00:00 2001 From: Clemens Backes Date: Tue, 27 Apr 2021 15:03:59 +0200 Subject: [PATCH] [Backport] Security bug 1201340 Manual cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/v8/v8/+/2875210: Merged: [liftoff] Fix >=2GB memory accesses on 32-bit We were inconsistent in handling offsets >= 2GB on 32-bit systems. The code was still relying on this being detected as statically out of bounds, but with the increase of {kV8MaxWasmMemoryPages} to support 4GB memories, this is not the case any more. This CL fixes this by again detecting such situations as statically OOB. We do not expect to be able to allocate memories of size >2GB on such systems. If this assumptions turns out to be wrong, we will erroneously trap. If that happens, we will have to explicitly disallow memories of such size on 32-bit systems. Tbr: jkummerow@chromium.org (cherry picked from commit 7ad5b961553d7d9bc30da1bb839726be2b92bb51) Bug: v8:7881, chromium:1201340 Change-Id: I8a91dd067a1c63a6d1caacb874a27b44b0983774 No-Try: true No-Presubmit: true No-Tree-Checks: true Reviewed-by: Clemens Backes Commit-Queue: Clemens Backes Cr-Commit-Position: refs/branch-heads/9.0@{#51} Cr-Branched-From: bd0108b4c88e0d6f2350cb79b5f363fbd02f3eb7-refs/heads/9.0.257@{#1} Cr-Branched-From: 349bcc6a075411f1a7ce2d866c3dfeefc2efa39d-refs/heads/master@{#73001} Reviewed-by: Allan Sandfeld Jensen --- .../wasm/baseline/arm/liftoff-assembler-arm.h | 18 ++++-------------- .../baseline/ia32/liftoff-assembler-ia32.h | 10 +++------- .../v8/src/wasm/baseline/liftoff-compiler.cc | 3 --- chromium/v8/src/wasm/compilation-environment.h | 8 +++++--- 4 files changed, 12 insertions(+), 27 deletions(-) diff --git a/chromium/v8/src/wasm/baseline/arm/liftoff-assembler-arm.h b/chromium/v8/src/wasm/baseline/arm/liftoff-assembler-arm.h index 9379a3b78a2..c6b4cc1276c 100644 --- a/chromium/v8/src/wasm/baseline/arm/liftoff-assembler-arm.h +++ b/chromium/v8/src/wasm/baseline/arm/liftoff-assembler-arm.h @@ -689,13 +689,8 @@ void LiftoffAssembler::Load(LiftoffRegister dst, Register src_addr, Register offset_reg, uint32_t offset_imm, LoadType type, LiftoffRegList pinned, uint32_t* protected_load_pc, bool is_load_mem) { - // If offset_imm cannot be converted to int32 safely, we abort as a separate - // check should cause this code to never be executed. - // TODO(7881): Support when >2GB is required. - if (!is_uint31(offset_imm)) { - TurboAssembler::Abort(AbortReason::kOffsetOutOfRange); - return; - } + // Offsets >=2GB are statically OOB on 32-bit systems. + DCHECK_LE(offset_imm, std::numeric_limits::max()); liftoff::LoadInternal(this, dst, src_addr, offset_reg, static_cast(offset_imm), type, pinned, protected_load_pc, is_load_mem); @@ -705,13 +700,8 @@ void LiftoffAssembler::Store(Register dst_addr, Register offset_reg, uint32_t offset_imm, LiftoffRegister src, StoreType type, LiftoffRegList pinned, uint32_t* protected_store_pc, bool is_store_mem) { - // If offset_imm cannot be converted to int32 safely, we abort as a separate - // check should cause this code to never be executed. - // TODO(7881): Support when >2GB is required. - if (!is_uint31(offset_imm)) { - TurboAssembler::Abort(AbortReason::kOffsetOutOfRange); - return; - } + // Offsets >=2GB are statically OOB on 32-bit systems. + DCHECK_LE(offset_imm, std::numeric_limits::max()); UseScratchRegisterScope temps(this); if (type.value() == StoreType::kF64Store) { Register actual_dst_addr = liftoff::CalculateActualAddress( diff --git a/chromium/v8/src/wasm/baseline/ia32/liftoff-assembler-ia32.h b/chromium/v8/src/wasm/baseline/ia32/liftoff-assembler-ia32.h index 9bfad9313b9..9005fe4303a 100644 --- a/chromium/v8/src/wasm/baseline/ia32/liftoff-assembler-ia32.h +++ b/chromium/v8/src/wasm/baseline/ia32/liftoff-assembler-ia32.h @@ -324,13 +324,8 @@ void LiftoffAssembler::Load(LiftoffRegister dst, Register src_addr, Register offset_reg, uint32_t offset_imm, LoadType type, LiftoffRegList pinned, uint32_t* protected_load_pc, bool is_load_mem) { - if (offset_imm > static_cast(std::numeric_limits::max())) { - // We do not generate code here, because such an offset should never pass - // the bounds check. However, the spec requires us to compile code with such - // an offset. - Trap(); - return; - } + // Offsets >=2GB are statically OOB on 32-bit systems. + DCHECK_LE(offset_imm, std::numeric_limits::max()); DCHECK_EQ(type.value_type() == kWasmI64, dst.is_gp_pair()); Operand src_op = offset_reg == no_reg ? Operand(src_addr, offset_imm) @@ -406,6 +401,7 @@ void LiftoffAssembler::Store(Register dst_addr, Register offset_reg, StoreType type, LiftoffRegList pinned, uint32_t* protected_store_pc, bool is_store_mem) { DCHECK_EQ(type.value_type() == kWasmI64, src.is_gp_pair()); + // Offsets >=2GB are statically OOB on 32-bit systems. DCHECK_LE(offset_imm, std::numeric_limits::max()); Operand dst_op = offset_reg == no_reg ? Operand(dst_addr, offset_imm) diff --git a/chromium/v8/src/wasm/baseline/liftoff-compiler.cc b/chromium/v8/src/wasm/baseline/liftoff-compiler.cc index c9625b06d8a..8103f5b7c14 100644 --- a/chromium/v8/src/wasm/baseline/liftoff-compiler.cc +++ b/chromium/v8/src/wasm/baseline/liftoff-compiler.cc @@ -2100,10 +2100,7 @@ class LiftoffCompiler { bool BoundsCheckMem(FullDecoder* decoder, uint32_t access_size, uint64_t offset, Register index, LiftoffRegList pinned, ForceCheck force_check) { - // If the offset does not fit in a uintptr_t, this can never succeed on this - // machine. const bool statically_oob = - offset > std::numeric_limits::max() || !base::IsInBounds(offset, access_size, env_->max_memory_size); diff --git a/chromium/v8/src/wasm/compilation-environment.h b/chromium/v8/src/wasm/compilation-environment.h index d730161ad1f..fc9b2fcbc2e 100644 --- a/chromium/v8/src/wasm/compilation-environment.h +++ b/chromium/v8/src/wasm/compilation-environment.h @@ -63,9 +63,11 @@ struct CompilationEnv { const LowerSimd lower_simd; - static constexpr uint32_t kMaxMemoryPagesAtRuntime = - std::min(kV8MaxWasmMemoryPages, - std::numeric_limits::max() / kWasmPageSize); + // We assume that memories of size >= half of the virtual address space + // cannot be allocated (see https://crbug.com/1201340). + static constexpr uint32_t kMaxMemoryPagesAtRuntime = std::min( + kV8MaxWasmMemoryPages, + (uintptr_t{1} << (kSystemPointerSize == 4 ? 31 : 63)) / kWasmPageSize); constexpr CompilationEnv(const WasmModule* module, UseTrapHandler use_trap_handler,