Security: V8: JIT: Simplified-lowererer IrOpcode::kStoreField, IrOpcode::kStoreElement optimization bug |
|||||||||||||||||||
Issue descriptionI 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.
,
Dec 2 2017
,
Dec 3 2017
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
,
Dec 4 2017
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?
,
Dec 4 2017
I think it could lead to an exploitable use-after-free condition. So I would set it High.
,
Dec 4 2017
,
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
,
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
,
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
,
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
,
Dec 7 2017
,
Dec 7 2017
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
,
Dec 7 2017
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
,
Dec 7 2017
,
Dec 7 2017
+awhalley@ for M63 merge review
,
Dec 8 2017
Approving merge for M64. Branch:3282
,
Dec 12 2017
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
,
Dec 13 2017
Please merge this issue to M64 branch 3282 if it has been verified in canary. The sooner the better. Thanks!
,
Dec 14 2017
I am a bit confused, this has already been merged to 6.4 (see comment #17).
,
Dec 14 2017
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.
,
Dec 15 2017
,
Jan 2 2018
It is probably not worth it to merge back to 6.3 right now. I don't think any more pushes are planned.
,
Jan 22 2018
,
Mar 2 2018
,
Mar 9 2018
,
Mar 15 2018
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
,
Mar 27 2018
|
|||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||
Comment 1 by elawrence@chromium.org
, Dec 2 2017