New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: V8: JIT: Simplified-lowererer IrOpcode::kStoreField, IrOpcode::kStoreElement optimization bug

Project Member Reported by lokihardt@google.com, Dec 2 2017

Issue description

I think this commit has introduced the bugs: https://chromium.googlesource.com/v8/v8/+/c22ca7f73ba92f22d0cd29b06bb2944a545a8d3e%5E%21/#F0

Here's a snippet.
  case IrOpcode::kStoreField: {
    FieldAccess access = FieldAccessOf(node->op());
    Node* value_node = node->InputAt(1);
    NodeInfo* input_info = GetInfo(value_node);
    MachineRepresentation field_representation =
        access.machine_type.representation();

    // Make sure we convert to Smi if possible. This should help write
    // barrier elimination.
    if (field_representation == MachineRepresentation::kTagged &&
        TypeOf(value_node)->Is(Type::SignedSmall())) {
      field_representation = MachineRepresentation::kTaggedSigned;
    }
    WriteBarrierKind write_barrier_kind = WriteBarrierKindFor(
        access.base_is_tagged, field_representation, access.offset,
        access.type, input_info->representation(), value_node);

    ProcessInput(node, 0, UseInfoForBasePointer(access));
    ProcessInput(node, 1,
                 TruncatingUseInfoFromRepresentation(field_representation));
    ProcessRemainingInputs(node, 2);
    SetOutput(node, MachineRepresentation::kNone);
    if (lower()) {
      if (write_barrier_kind < access.write_barrier_kind) {
        access.write_barrier_kind = write_barrier_kind;
        NodeProperties::ChangeOp(
            node, jsgraph_->simplified()->StoreField(access));
      }
    }
    return;
  }

Since Smi stores can be performed without write barriers, if it's possible to convert to Smi, it tries to help write barrier elimination by changing field_representation to MachineRepresentation::kTaggedSigned as noted in the comment. But whether or not field_representation has changed, it uses TruncatingUseInfoFromRepresentation to process the value node.

But TruncatingUseInfoFromRepresentation(kTaggedSigned) returns UseInfo::AnyTagged() which is also compatible with kTaggedPointer. So even in the case where input_info->representation() is kTaggedPointer and the value is a heap object, it may eliminate the write barrier.

Note: It's the same when handling kStoreElement.

PoC 1 using kStoreField.
var a, b;  // should be var
for (var i = 0; i < 100000; i++) {
    b = 1;
    a = i + -0;  // -0 is a number, so this will make "a" a heap object.
    b = a;
}

print(a === b);  // true
gc();
print(a === b);  // false
print(b);

PoC 2 using kStoreElement.
let arr = [{}];
var v;  // should be var
for (var i = 0; i < 700000; i++) {
    arr[0] = 1;
    v = i + -0;
    arr[0] = v;
}

print(arr[0] === v)  // true
gc();
print(arr[0] === v)  // false
print(arr[0]);


This bug is subject to a 90 day disclosure deadline. After 90 days elapse
or a patch has been made broadly available, the bug report will become
visible to the public.

 
Components: Blink>JavaScript>Compiler
Cc: mvstan...@chromium.org
Owner: jarin@chromium.org
Status: Assigned (was: Unconfirmed)
Labels: Security_Impact-Stable M-62
It's unclear what the severity of this bug is. jarin: could you please estimate the severity based on https://chromium.googlesource.com/chromium/src/+/master/docs/security/severity-guidelines.md ? 

Thanks
Labels: Security_Severity-Medium OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
At a minimum, we'd seem to have application-semantic bugs, depending on the calling application's behavior and expectations. I'm trying to figure out of there could also be type confusion inside V8 as a result of this? The severity would seem to be at least Medium, but if there's type confusion inside V8, that'd be High. Thoughts, lokihardt?
I think it could lead to an exploitable use-after-free condition. So I would set it High.
Labels: -Security_Severity-Medium Security_Severity-High
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 5 2017

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

commit cc07ac73a460de3d933ebe7aebf0f55b741eb5cf
Author: Jaroslav Sevcik <jarin@chromium.org>
Date: Tue Dec 05 06:00:57 2017

[turbofan] Make sure TruncatingUseInfoFromRepresentation respects Smi representation.

Eventually, we want to fix this also for tagged pointers (tracking bug: https://crbug.com/v8/7162).

Bug:  chromium:791245 
Change-Id: I93d6deff36cedcc9a4665fab0abe6fffdae9b61b
Reviewed-on: https://chromium-review.googlesource.com/806457
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49850}
[modify] https://crrev.com/cc07ac73a460de3d933ebe7aebf0f55b741eb5cf/src/compiler/js-call-reducer.cc
[modify] https://crrev.com/cc07ac73a460de3d933ebe7aebf0f55b741eb5cf/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/cc07ac73a460de3d933ebe7aebf0f55b741eb5cf/src/compiler/simplified-lowering.cc
[add] https://crrev.com/cc07ac73a460de3d933ebe7aebf0f55b741eb5cf/test/mjsunit/compiler/regress-791245.js

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 5 2017

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

commit 897416af7f5b6e974536bce9abd398789abcccf5
Author: Michael Achenbach <machenbach@chromium.org>
Date: Tue Dec 05 08:27:35 2017

Revert "[turbofan] Make sure TruncatingUseInfoFromRepresentation respects Smi representation."

This reverts commit cc07ac73a460de3d933ebe7aebf0f55b741eb5cf.

Reason for revert: Breaks benchmarks:
http://shortn/_POjH6zA7tp

Original change's description:
> [turbofan] Make sure TruncatingUseInfoFromRepresentation respects Smi representation.
> 
> Eventually, we want to fix this also for tagged pointers (tracking bug: https://crbug.com/v8/7162).
> 
> Bug:  chromium:791245 
> Change-Id: I93d6deff36cedcc9a4665fab0abe6fffdae9b61b
> Reviewed-on: https://chromium-review.googlesource.com/806457
> Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
> Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#49850}

TBR=jarin@chromium.org,bmeurer@chromium.org

Change-Id: I0ff571b161ec40ba1f32ee048f8255c42414d8d2
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:791245 
Reviewed-on: https://chromium-review.googlesource.com/807985
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49853}
[modify] https://crrev.com/897416af7f5b6e974536bce9abd398789abcccf5/src/compiler/js-call-reducer.cc
[modify] https://crrev.com/897416af7f5b6e974536bce9abd398789abcccf5/src/compiler/js-native-context-specialization.cc
[modify] https://crrev.com/897416af7f5b6e974536bce9abd398789abcccf5/src/compiler/simplified-lowering.cc
[delete] https://crrev.com/b501bf936e73e157062409fa8e2c71c501490ed2/test/mjsunit/compiler/regress-791245.js

Project Member

Comment 9 by bugdroid1@chromium.org, Dec 5 2017

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

commit 3ef6e45ee3a59a50d7cd2c9b6347f3b7323adbc5
Author: Benedikt Meurer <bmeurer@chromium.org>
Date: Tue Dec 05 09:51:15 2017

[turbofan] Properly type the OrderedHashTableHealIndex builtin result.

This unblocks the checks in the SimplifiedLowering that whenever we
store something as TaggedSigned, the input type should at least be
Type::SignedSmall.

Bug:  chromium:791245 
Change-Id: Ice6e55c2c6584c0ff60c1e033ba755c8863af32a
Reviewed-on: https://chromium-review.googlesource.com/808104
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49856}
[modify] https://crrev.com/3ef6e45ee3a59a50d7cd2c9b6347f3b7323adbc5/src/compiler/js-builtin-reducer.cc
[add] https://crrev.com/3ef6e45ee3a59a50d7cd2c9b6347f3b7323adbc5/test/mjsunit/regress/regress-crbug-791245-1.js
[add] https://crrev.com/3ef6e45ee3a59a50d7cd2c9b6347f3b7323adbc5/test/mjsunit/regress/regress-crbug-791245-2.js

Project Member

Comment 10 by bugdroid1@chromium.org, Dec 5 2017

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

commit f8834852a973d6cd3b20c28bb73ef64a57f8acf7
Author: Jaroslav Sevcik <jarin@chromium.org>
Date: Tue Dec 05 14:26:46 2017

[turbofan] Temporarily disable write barrier elimination for stores of small integers.

The proper fix would be to make TruncatingUseInfoFromRepresentation
respect tagged signed use representation, but requires extra work
to refine typing for all values that are stored into Smi fields.

Bug:  chromium:791245 
Change-Id: I83965bcc18a836d2c758a6a8b1477a4aa2c6133d
Reviewed-on: https://chromium-review.googlesource.com/808866
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49870}
[modify] https://crrev.com/f8834852a973d6cd3b20c28bb73ef64a57f8acf7/src/compiler/simplified-lowering.cc
[add] https://crrev.com/f8834852a973d6cd3b20c28bb73ef64a57f8acf7/test/mjsunit/compiler/regress-791245.js

Labels: Merge-Request-64 Merge-Request-63
Status: Fixed (was: Assigned)
Project Member

Comment 12 by sheriffbot@chromium.org, Dec 7 2017

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

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

Comment 13 by sheriffbot@chromium.org, Dec 7 2017

Labels: -Merge-Request-63 Merge-Review-63
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), gkihumba@(ChromeOS), govind@(Desktop)

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

Comment 14 by sheriffbot@chromium.org, Dec 7 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Cc: awhalley@chromium.org
+awhalley@ for M63 merge review
Labels: -Merge-Review-64 Merge-Approved-64
Approving merge for M64. Branch:3282
Project Member

Comment 17 by bugdroid1@chromium.org, Dec 12 2017

Labels: merge-merged-6.4
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/a27b116b1aea69e9cf8752e4e5a5900675401e2c

commit a27b116b1aea69e9cf8752e4e5a5900675401e2c
Author: Jaroslav Sevcik <jarin@chromium.org>
Date: Tue Dec 12 08:53:00 2017

Merged: [turbofan] Temporarily disable write barrier elimination for stores of small integers.

Revision: f8834852a973d6cd3b20c28bb73ef64a57f8acf7

BUG= chromium:791245 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=bmeurer@chromium.org

Change-Id: I442c5c745df2e1c62f344945ba79f09033fcd6d5
Reviewed-on: https://chromium-review.googlesource.com/817440
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/branch-heads/6.4@{#12}
Cr-Branched-From: 0407506af3d9d7e2718be1d8759296165b218fcf-refs/heads/6.4.388@{#1}
Cr-Branched-From: a5fc4e085ee543cb608eb11034bc8f147ba388e1-refs/heads/master@{#49724}
[modify] https://crrev.com/a27b116b1aea69e9cf8752e4e5a5900675401e2c/src/compiler/simplified-lowering.cc
[add] https://crrev.com/a27b116b1aea69e9cf8752e4e5a5900675401e2c/test/mjsunit/compiler/regress-791245.js

Please merge this issue to M64 branch 3282 if it has been verified in canary. The sooner the better. Thanks!

Comment 19 by jarin@chromium.org, Dec 14 2017

I am a bit confused, this has already been merged to 6.4 (see comment #17).
All good jarin@ and sorry for the confusion.

Bugdroid should be able to remove Merge-Approved-64 once merge-merged-6.4 is applied. I will look into why this is not the case.


Labels: -Merge-Approved-64
Labels: -Merge-Review-63 Merge-Rejected-63
It is probably not worth it to merge back to 6.3 right now. I don't think any more pushes are planned.
Labels: Release-0-M64
Cc: pelizzi@google.com
Cc: neis@chromium.org
Project Member

Comment 26 by sheriffbot@chromium.org, Mar 15

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
Project Member

Comment 27 by sheriffbot@chromium.org, Mar 27

Labels: -M-62 M-65

Sign in to add a comment