Skip to content

Commit

Permalink
[Backport] Security bug 1201340
Browse files Browse the repository at this point in the history
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: [email protected]

(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 <[email protected]>
Commit-Queue: Clemens Backes <[email protected]>
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 <[email protected]>
  • Loading branch information
backes authored and mibrunin committed May 28, 2021
1 parent 9827f0c commit 951cdb3
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 27 deletions.
18 changes: 4 additions & 14 deletions chromium/v8/src/wasm/baseline/arm/liftoff-assembler-arm.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<int32_t>::max());
liftoff::LoadInternal(this, dst, src_addr, offset_reg,
static_cast<int32_t>(offset_imm), type, pinned,
protected_load_pc, is_load_mem);
Expand All @@ -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<int32_t>::max());
UseScratchRegisterScope temps(this);
if (type.value() == StoreType::kF64Store) {
Register actual_dst_addr = liftoff::CalculateActualAddress(
Expand Down
10 changes: 3 additions & 7 deletions chromium/v8/src/wasm/baseline/ia32/liftoff-assembler-ia32.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint32_t>(std::numeric_limits<int32_t>::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<int32_t>::max());
DCHECK_EQ(type.value_type() == kWasmI64, dst.is_gp_pair());
Operand src_op = offset_reg == no_reg
? Operand(src_addr, offset_imm)
Expand Down Expand Up @@ -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<int32_t>::max());
Operand dst_op = offset_reg == no_reg
? Operand(dst_addr, offset_imm)
Expand Down
3 changes: 0 additions & 3 deletions chromium/v8/src/wasm/baseline/liftoff-compiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<uintptr_t>::max() ||
!base::IsInBounds<uintptr_t>(offset, access_size,
env_->max_memory_size);

Expand Down
8 changes: 5 additions & 3 deletions chromium/v8/src/wasm/compilation-environment.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,11 @@ struct CompilationEnv {

const LowerSimd lower_simd;

static constexpr uint32_t kMaxMemoryPagesAtRuntime =
std::min(kV8MaxWasmMemoryPages,
std::numeric_limits<uintptr_t>::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,
Expand Down

0 comments on commit 951cdb3

Please sign in to comment.