New issue
Advanced search Search tips

Issue 894374 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 12
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug-Security

Blocking:
issue 894307



Sign in to add a comment

[liftoff] [ia32] Debug check failed: !unpinned.is_empty()

Project Member Reported by clemensh@chromium.org, Oct 11

Issue description

Found by fuzzing locally (we still don't have automated ia32 fuzzers):

load('test/mjsunit/wasm/wasm-constants.js');
load('test/mjsunit/wasm/wasm-module-builder.js');

const builder = new WasmModuleBuilder();
builder.addMemory(16, 32, false);
const sig = makeSig([kWasmI32, kWasmI32, kWasmI32], [kWasmI32]);
builder.addFunction(undefined, sig)
  .addBodyWithEnd([
    kExprMemorySize, 0,
    kExprI32Const, 0,
    kExprI64Const, 0,
    kExprI64StoreMem8, 0, 0,
    kExprEnd,   // @16
  ]);
builder.addExport('main', 0);
builder.instantiate();

Results in:

#                                                                                                       
# Fatal error in ../../src/wasm/baseline/liftoff-assembler.h, line 218                     
# Debug check failed: !unpinned.is_empty().                                                                                                                          
#                                                                                          

This was caused by removing the ebx register (CL: https://crrev.com/c/1270589).
Will try to fix this by reducing the number of live registers.

Marking as security bug, as only debug checks will fail, and we might generate illegal code.
 
Blocking: 894307
Project Member

Comment 2 by sheriffbot@chromium.org, Oct 11

Labels: -Pri-1 Pri-2
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 12

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/56b8ab5d07a7f9300f20d06b68c34237b3be2d1b

commit 56b8ab5d07a7f9300f20d06b68c34237b3be2d1b
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Fri Oct 12 08:11:52 2018

[Liftoff] Fewer pinned registers on store

On ia32, we were pinning too many registers, resulting in no unpinned
byte registers left (we only have three byte registers since {ebx}
is reserved for the root register).
It turns out that on most paths, we don't actually need to pin any
registers, since {Store} is often the last call for an operation (like
any store or set_global). If registers need to be pinned, only pass
those that must be kept alive across the {Store}. This allows to
compute a more narrow set of pinned registers on demand inside {Store}.

Plus minor drive-by changes.

R=titzer@chromium.org

Bug:  chromium:894374 , chromium:894307, v8:6600
Change-Id: Ic4d7131784c193dc7a2abf0e504d9973f6d5c5f1
Reviewed-on: https://chromium-review.googlesource.com/c/1275819
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#56587}
[modify] https://crrev.com/56b8ab5d07a7f9300f20d06b68c34237b3be2d1b/src/wasm/baseline/ia32/liftoff-assembler-ia32.h
[modify] https://crrev.com/56b8ab5d07a7f9300f20d06b68c34237b3be2d1b/src/wasm/baseline/liftoff-compiler.cc
[modify] https://crrev.com/56b8ab5d07a7f9300f20d06b68c34237b3be2d1b/src/wasm/baseline/liftoff-register.h
[modify] https://crrev.com/56b8ab5d07a7f9300f20d06b68c34237b3be2d1b/src/wasm/baseline/mips/liftoff-assembler-mips.h
[modify] https://crrev.com/56b8ab5d07a7f9300f20d06b68c34237b3be2d1b/src/wasm/baseline/mips64/liftoff-assembler-mips64.h
[modify] https://crrev.com/56b8ab5d07a7f9300f20d06b68c34237b3be2d1b/src/wasm/baseline/x64/liftoff-assembler-x64.h
[add] https://crrev.com/56b8ab5d07a7f9300f20d06b68c34237b3be2d1b/test/mjsunit/regress/wasm/regress-894374.js

Status: Fixed (was: Started)
Project Member

Comment 5 by sheriffbot@chromium.org, Oct 12

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 6 by sheriffbot@chromium.org, Jan 18 (4 days ago)

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment