From 1461ea96319c7ed5eb5323ba17749b57a6825832 Mon Sep 17 00:00:00 2001 From: Tobias Tebbi Date: Fri, 17 Jun 2022 10:14:44 +0000 Subject: [Backport] CVE-2022-2295: Type Confusion in V8 Partial cherry-pick of patch originally reviewed on https://chromium-review.googlesource.com/c/v8/v8/+/3726007: make CanCover() transitive In addition to checking that a node is owned, CanCover() also needs to check if there are any side-effects in between the current node and the merged node. When merging inputs of inputs, this check was done with the wrong side-effect level of the in-between node. We partially fixed this before with `CanCoverTransitively`. This CL addresses the issue by always comparing to the side-effect level of the node from which we started, making `CanCoverTransitively` superfluous. (cherry picked from commit 6048f754931e0971cab58fb0de785482d175175b) Bug: chromium:1336869 No-Try: true No-Presubmit: true No-Tree-Checks: true Change-Id: I78479b32461ede81138f8b5d48d60058cfb5fa0a Commit-Queue: Tobias Tebbi Cr-Original-Commit-Position: refs/heads/main@{#81217} Commit-Queue: Roger Felipe Zanoni da Silva Reviewed-by: Artem Sumaneev Reviewed-by: Tobias Tebbi Owners-Override: Tobias Tebbi Cr-Commit-Position: refs/branch-heads/9.6@{#70} Cr-Branched-From: 0b7bda016178bf438f09b3c93da572ae3663a1f7-refs/heads/9.6.180@{#1} Cr-Branched-From: 41a5a247d9430b953e38631e88d17790306f7a4c-refs/heads/main@{#77244} Reviewed-by: Michal Klocek --- .../v8/src/compiler/backend/instruction-selector.cc | 21 ++++----------------- .../v8/src/compiler/backend/instruction-selector.h | 14 ++++++-------- .../backend/mips64/instruction-selector-mips64.cc | 2 +- .../backend/riscv64/instruction-selector-riscv64.cc | 2 +- .../backend/x64/instruction-selector-x64.cc | 2 +- 5 files changed, 13 insertions(+), 28 deletions(-) diff --git a/chromium/v8/src/compiler/backend/instruction-selector.cc b/chromium/v8/src/compiler/backend/instruction-selector.cc index 92c11e09b44..27a5d85b1b9 100644 --- a/chromium/v8/src/compiler/backend/instruction-selector.cc +++ b/chromium/v8/src/compiler/backend/instruction-selector.cc @@ -284,7 +284,7 @@ Instruction* InstructionSelector::Emit(Instruction* instr) { bool InstructionSelector::CanCover(Node* user, Node* node) const { // 1. Both {user} and {node} must be in the same basic block. - if (schedule()->block(node) != schedule()->block(user)) { + if (schedule()->block(node) != current_block_) { return false; } // 2. Pure {node}s must be owned by the {user}. @@ -292,7 +292,7 @@ bool InstructionSelector::CanCover(Node* user, Node* node) const { return node->OwnedBy(user); } // 3. Impure {node}s must match the effect level of {user}. - if (GetEffectLevel(node) != GetEffectLevel(user)) { + if (GetEffectLevel(node) != current_effect_level_) { return false; } // 4. Only {node} must have value edges pointing to {user}. @@ -304,21 +304,6 @@ bool InstructionSelector::CanCover(Node* user, Node* node) const { return true; } -bool InstructionSelector::CanCoverTransitively(Node* user, Node* node, - Node* node_input) const { - if (CanCover(user, node) && CanCover(node, node_input)) { - // If {node} is pure, transitivity might not hold. - if (node->op()->HasProperty(Operator::kPure)) { - // If {node_input} is pure, the effect levels do not matter. - if (node_input->op()->HasProperty(Operator::kPure)) return true; - // Otherwise, {user} and {node_input} must have the same effect level. - return GetEffectLevel(user) == GetEffectLevel(node_input); - } - return true; - } - return false; -} - bool InstructionSelector::IsOnlyUserOfNodeInSameBlock(Node* user, Node* node) const { BasicBlock* bb_user = schedule()->block(user); @@ -1226,6 +1211,7 @@ void InstructionSelector::VisitBlock(BasicBlock* block) { int effect_level = 0; for (Node* const node : *block) { SetEffectLevel(node, effect_level); + current_effect_level_ = effect_level; if (node->opcode() == IrOpcode::kStore || node->opcode() == IrOpcode::kUnalignedStore || node->opcode() == IrOpcode::kCall || @@ -1245,6 +1231,7 @@ void InstructionSelector::VisitBlock(BasicBlock* block) { // control input should be on the same effect level as the last node. if (block->control_input() != nullptr) { SetEffectLevel(block->control_input(), effect_level); + current_effect_level_ = effect_level; } auto FinishEmittedInstructions = [&](Node* node, int instruction_start) { diff --git a/chromium/v8/src/compiler/backend/instruction-selector.h b/chromium/v8/src/compiler/backend/instruction-selector.h index 11a329d1d6e..f8ede1f37ff 100644 --- a/chromium/v8/src/compiler/backend/instruction-selector.h +++ b/chromium/v8/src/compiler/backend/instruction-selector.h @@ -452,15 +452,12 @@ class V8_EXPORT_PRIVATE InstructionSelector final { // Used in pattern matching during code generation. // Check if {node} can be covered while generating code for the current // instruction. A node can be covered if the {user} of the node has the only - // edge and the two are in the same basic block. - // Before fusing two instructions a and b, it is useful to check that - // CanCover(a, b) holds. If this is not the case, code for b must still be - // generated for other users, and fusing is unlikely to improve performance. + // edge, the two are in the same basic block, and there are no side-effects + // in-between. The last check is crucial for soundness. + // For pure nodes, CanCover(a,b) is checked to avoid duplicated execution: + // If this is not the case, code for b must still be generated for other + // users, and fusing is unlikely to improve performance. bool CanCover(Node* user, Node* node) const; - // CanCover is not transitive. The counter example are Nodes A,B,C such that - // CanCover(A, B) and CanCover(B,C) and B is pure: The the effect level of A - // and B might differ. CanCoverTransitively does the additional checks. - bool CanCoverTransitively(Node* user, Node* node, Node* node_input) const; // Used in pattern matching during code generation. // This function checks that {node} and {user} are in the same basic block, @@ -787,6 +784,7 @@ class V8_EXPORT_PRIVATE InstructionSelector final { BoolVector defined_; BoolVector used_; IntVector effect_level_; + int current_effect_level_; IntVector virtual_registers_; IntVector virtual_register_rename_; InstructionScheduler* scheduler_; diff --git a/chromium/v8/src/compiler/backend/mips64/instruction-selector-mips64.cc b/chromium/v8/src/compiler/backend/mips64/instruction-selector-mips64.cc index bec7bbefdcb..42c00365037 100644 --- a/chromium/v8/src/compiler/backend/mips64/instruction-selector-mips64.cc +++ b/chromium/v8/src/compiler/backend/mips64/instruction-selector-mips64.cc @@ -1527,7 +1527,7 @@ void InstructionSelector::VisitTruncateInt64ToInt32(Node* node) { if (CanCover(node, value)) { switch (value->opcode()) { case IrOpcode::kWord64Sar: { - if (CanCoverTransitively(node, value, value->InputAt(0)) && + if (CanCover(value, value->InputAt(0)) && TryEmitExtendingLoad(this, value, node)) { return; } else { diff --git a/chromium/v8/src/compiler/backend/riscv64/instruction-selector-riscv64.cc b/chromium/v8/src/compiler/backend/riscv64/instruction-selector-riscv64.cc index 72706201e2a..210add8dfb6 100644 --- a/chromium/v8/src/compiler/backend/riscv64/instruction-selector-riscv64.cc +++ b/chromium/v8/src/compiler/backend/riscv64/instruction-selector-riscv64.cc @@ -1323,7 +1323,7 @@ void InstructionSelector::VisitTruncateInt64ToInt32(Node* node) { if (CanCover(node, value)) { switch (value->opcode()) { case IrOpcode::kWord64Sar: { - if (CanCoverTransitively(node, value, value->InputAt(0)) && + if (CanCover(value, value->InputAt(0)) && TryEmitExtendingLoad(this, value, node)) { return; } else { diff --git a/chromium/v8/src/compiler/backend/x64/instruction-selector-x64.cc b/chromium/v8/src/compiler/backend/x64/instruction-selector-x64.cc index 53ee75064bb..5c86dbc0c69 100644 --- a/chromium/v8/src/compiler/backend/x64/instruction-selector-x64.cc +++ b/chromium/v8/src/compiler/backend/x64/instruction-selector-x64.cc @@ -1729,7 +1729,7 @@ void InstructionSelector::VisitTruncateInt64ToInt32(Node* node) { case IrOpcode::kWord64Shr: { Int64BinopMatcher m(value); if (m.right().Is(32)) { - if (CanCoverTransitively(node, value, value->InputAt(0)) && + if (CanCover(value, value->InputAt(0)) && TryMatchLoadWord64AndShiftRight(this, value, kX64Movl)) { return EmitIdentity(node); } -- cgit v1.2.1