cfallin opened PR #1930 from spectre-heap
to main
:
This PR adds a conditional move following a heap bounds check through
which the address to be accessed flows. This conditional move ensures
that even if the branch is mispredicted (access is actually out of
bounds, but speculation goes down in-bounds path), the acually accessed
address is zero (a NULL pointer) rather than the out-of-bounds address.The technique used is well-known and fairly standard: see, e.g., SpiderMonkey's heap access bounds check (thanks to @bnjbvr for locating this).
The mitigation is controlled by a flag that is off by default, but can
be set by the embedding, to avoid unexpected performance impact for
existing users.Note that the mitigation is unneccessary when we use the "huge heap"
technique on 64-bit systems, in which we allocate a range of virtual
address space such that no 32-bit offset can reach other data. Hence,
this only affects small-heap configurations.The generated code is not great on aarch64 because the overflow flag must be materialized as a boolean in a register, rather than reusing flags (because the dataflow crosses basic blocks); in theory we could do much better with a later lowering, codegenning one comparison and two ops that use its flags.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin edited PR #1930 from spectre-heap
to main
:
This PR adds a conditional move following a heap bounds check through which the address to be accessed flows. This conditional move ensures that even if the branch is mispredicted (access is actually out of bounds, but speculation goes down in-bounds path), the acually accessed address is zero (a NULL pointer) rather than the out-of-bounds address.
The technique used is well-known and fairly standard: see, e.g., SpiderMonkey's heap access bounds check (thanks to @bnjbvr for locating this).
The mitigation is controlled by a flag that is off by default, but can be set by the embedding, to avoid unexpected performance impact for existing users.
Note that the mitigation is unneccessary when we use the "huge heap" technique on 64-bit systems, in which we allocate a range of virtual address space such that no 32-bit offset can reach other data. Hence, this only affects small-heap configurations.
The generated code is not great on aarch64 because the overflow flag must be materialized as a boolean in a register, rather than reusing flags (because the dataflow crosses basic blocks); in theory we could do much better with a later lowering, codegenning one comparison and two ops that use its flags.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin requested bnjbvr for a review on PR #1930.
bnjbvr submitted PR Review.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
Generating IR for the Spectre check seems quite dangerous, at this point; any optimization could reorganize the code (move it around in the best case, delete it in the worst case), so I don't think this is the right solution here.
bnjbvr created PR Review Comment:
The risk of causing harm by disabling Spectre mitigations by default seems much worse than the performance hit that will be caused by its being set all the time, so I think we should enable it by default, explicit the performance overhead here, and say that while it's enabled by default, there are certain environments in which it's fine to disable it.
bjorn3 submitted PR Review.
bjorn3 created PR Review Comment:
Even worse, there is a legalization for
select
that uses branches when there is no encoding that usescmov
:
cfallin updated PR #1930 from spectre-heap
to main
:
This PR adds a conditional move following a heap bounds check through which the address to be accessed flows. This conditional move ensures that even if the branch is mispredicted (access is actually out of bounds, but speculation goes down in-bounds path), the acually accessed address is zero (a NULL pointer) rather than the out-of-bounds address.
The technique used is well-known and fairly standard: see, e.g., SpiderMonkey's heap access bounds check (thanks to @bnjbvr for locating this).
The mitigation is controlled by a flag that is off by default, but can be set by the embedding, to avoid unexpected performance impact for existing users.
Note that the mitigation is unneccessary when we use the "huge heap" technique on 64-bit systems, in which we allocate a range of virtual address space such that no 32-bit offset can reach other data. Hence, this only affects small-heap configurations.
The generated code is not great on aarch64 because the overflow flag must be materialized as a boolean in a register, rather than reusing flags (because the dataflow crosses basic blocks); in theory we could do much better with a later lowering, codegenning one comparison and two ops that use its flags.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin submitted PR Review.
cfallin created PR Review Comment:
Changed to use a specific, effectful instruction that the optimizer and legalizations do not know about.
cfallin submitted PR Review.
cfallin created PR Review Comment:
See below: will need to be
false
by default unless we also implement for the legacy x86 backend (or until we delete it), which (unless I'm mistaken) seems to be missing some of the necessary bits for cmovs.
cfallin updated PR #1930 from spectre-heap
to main
:
This PR adds a conditional move following a heap bounds check through which the address to be accessed flows. This conditional move ensures that even if the branch is mispredicted (access is actually out of bounds, but speculation goes down in-bounds path), the acually accessed address is zero (a NULL pointer) rather than the out-of-bounds address.
The technique used is well-known and fairly standard: see, e.g., SpiderMonkey's heap access bounds check (thanks to @bnjbvr for locating this).
The mitigation is controlled by a flag that is off by default, but can be set by the embedding, to avoid unexpected performance impact for existing users.
Note that the mitigation is unneccessary when we use the "huge heap" technique on 64-bit systems, in which we allocate a range of virtual address space such that no 32-bit offset can reach other data. Hence, this only affects small-heap configurations.
The generated code is not great on aarch64 because the overflow flag must be materialized as a boolean in a register, rather than reusing flags (because the dataflow crosses basic blocks); in theory we could do much better with a later lowering, codegenning one comparison and two ops that use its flags.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin submitted PR Review.
cfallin created PR Review Comment:
Now on by default with latest version.
bnjbvr submitted PR Review.
bnjbvr created PR Review Comment:
nit: for symmetry with the other function, can you use a destructuring assignment here too?
let (cc, offset, bound) = ...
bnjbvr submitted PR Review.
cfallin updated PR #1930 from spectre-heap
to main
:
This PR adds a conditional move following a heap bounds check through which the address to be accessed flows. This conditional move ensures that even if the branch is mispredicted (access is actually out of bounds, but speculation goes down in-bounds path), the acually accessed address is zero (a NULL pointer) rather than the out-of-bounds address.
The technique used is well-known and fairly standard: see, e.g., SpiderMonkey's heap access bounds check (thanks to @bnjbvr for locating this).
The mitigation is controlled by a flag that is off by default, but can be set by the embedding, to avoid unexpected performance impact for existing users.
Note that the mitigation is unneccessary when we use the "huge heap" technique on 64-bit systems, in which we allocate a range of virtual address space such that no 32-bit offset can reach other data. Hence, this only affects small-heap configurations.
The generated code is not great on aarch64 because the overflow flag must be materialized as a boolean in a register, rather than reusing flags (because the dataflow crosses basic blocks); in theory we could do much better with a later lowering, codegenning one comparison and two ops that use its flags.
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
cfallin submitted PR Review.
cfallin created PR Review Comment:
Done!
cfallin merged PR #1930.
Last updated: Dec 23 2024 at 12:05 UTC