New issue
Advanced search Search tips

Issue 864509 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 17
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Security

Blocking:
issue 862929



Sign in to add a comment

Liftoff must ensure that i32 stack parameters are zero extended

Project Member Reported by clemensh@chromium.org, Jul 17

Issue description

Liftoff on x64 spills values to the stack by using "movl", hence the upper 32 bit are arbitrary (stale data from the stack). The spill slots can later directly be used as call arguments, which might end up on the stack again, in this case Liftoff just emits something like "push [rbp-0x48]". Hence it copies the uninitialized 32 bit over to the call argument stack slot. If the called function is compiled by Turbofan, it might load the 32 bit argument using a 64-bit load. Hence we end up with a not-zero-extended 32-bit value. This is problematic if that value is passed back to Liftoff, which assumes zero-extension and might use the value directly as memory offset.

The easiest solution is to ensure that we zero extend all call argument stack slots.
Another solution (long-term) is to make Turbofan use "movl" to load 32-bit values from the stack.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 17

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

commit 16af1baac4bf02d7a80e22501afdb790c84fd80e
Author: Clemens Hammacher <clemensh@chromium.org>
Date: Tue Jul 17 16:59:14 2018

[Liftoff] Zero-extend i32 stack parameters

i32 stack parameters can be loaded by Turbofan as 64-bit value, hence
they would not be zero extended. If this loaded value is then passed to
Liftoff (which assumes zero-extended i32 values), we could use it for
memory accesses, which would be out of bounds.

R=mstarzinger@chromium.org

Bug:  chromium:864509 , v8:6600
Change-Id: I0f45a269b1fb1c2befc2e6bc660c559a88323767
Reviewed-on: https://chromium-review.googlesource.com/1140168
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54500}
[modify] https://crrev.com/16af1baac4bf02d7a80e22501afdb790c84fd80e/src/wasm/baseline/x64/liftoff-assembler-x64.h
[add] https://crrev.com/16af1baac4bf02d7a80e22501afdb790c84fd80e/test/mjsunit/regress/wasm/regress-864509.js

Status: Fixed (was: Started)
Project Member

Comment 3 by sheriffbot@chromium.org, Jul 18

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

Comment 5 by sheriffbot@chromium.org, Oct 24

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