cfallin opened PR #5972 from 64-bit-addresses-are-64-bits
to main
:
@avanhatt has been looking at our address-mode lowering and found an example where when feeding an
I32
-typed address into a load or store, we can violate assumptions and get incorrect codegen.This should never be reachable in practice, because all producers on 64-bit architectures use 64-bit types for addresses. However, our IR is insufficiently constrained, and allows loads/stores to
I32
addresses as well. This is nonsensical on a 64-bit architecture.Initially I had thought we should tighten either the instruction definition's accepted types, or the CLIF verifier, to reject this. However both are target-independent, and we don't want to bake an assumption of 64-bit-ness into the compiler core. Instead this PR tightens specific backends' lowerings to rejecct loads/stores of
I32
-typed addresses.An alternative would be to modify the definition of the instructions (the
iAddr
type set) to take onlyI64
s today, since all current backends are for 64-bit ISAs; or modify the verifier to require an ISA, and make verification an ISA-specific property. The former falls apart as soon as we get a 32-bit backend (and we don't want to close that door); the latter might work, but I'm not sure I like it any better. (In its favor, word size seems like the one machine property we might actually want to parameterize CLIF correctness on.)aarch64 still has
lower_address
in Rust and no clear single place to put an LHS constraint, so I opted for an assert in the external ctor. riscv64 has no single point where we have aValue
and produce an address (gen_load
takes aReg
) so all load/store lowerings get the constraint.tl;dr: no security implications as all producers use I64-typed addresses (and must, for correct operation); but we currently accept I32-typed addresses too, and this breaks other assumptions.
<!--
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 fitzgen for a review on PR #5972.
fitzgen submitted PR review.
cfallin edited PR #5972 from 64-bit-addresses-are-64-bits
to main
:
@avanhatt has been looking at our address-mode lowering and found an example where when feeding an
I32
-typed address into a load or store, we can violate assumptions and get incorrect codegen.This should never be reachable in practice, because all producers on 64-bit architectures use 64-bit types for addresses. However, our IR is insufficiently constrained, and allows loads/stores to
I32
addresses as well. This is nonsensical on a 64-bit architecture.Initially I had thought we should tighten either the instruction definition's accepted types, or the CLIF verifier, to reject this. However both are target-independent, and we don't want to bake an assumption of 64-bit-ness into the compiler core. Instead this PR tightens specific backends' lowerings to rejecct loads/stores of
I32
-typed addresses.aarch64 still has
lower_address
in Rust and no clear single place to put an LHS constraint, so I opted for an assert in the external ctor. riscv64 has no single point where we have aValue
and produce an address (gen_load
takes aReg
) so all load/store lowerings get the constraint.tl;dr: no security implications as all producers use I64-typed addresses (and must, for correct operation); but we currently accept I32-typed addresses too, and this breaks other assumptions.
<!--
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 updated PR #5972 from 64-bit-addresses-are-64-bits
to main
.
cfallin has enabled auto merge for PR #5972.
cfallin updated PR #5972 from 64-bit-addresses-are-64-bits
to main
.
cfallin has disabled auto merge for PR #5972.
cfallin requested fitzgen for a review on PR #5972.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
Does
ty_64
succeed forf64
? I guess the CLIF verifier would presumably catch that one, but it may make sense to use a purpose built extractor that is calledis_address_type
or something like that.
cfallin submitted PR review.
cfallin created PR review comment:
It looks like the instruction definition (the
iAddr
type set) includes only ints and reference types, so this case can't happen. I do likeis_address_type
though; I'll do that.
cfallin updated PR #5972 from 64-bit-addresses-are-64-bits
to main
.
cfallin has enabled auto merge for PR #5972.
avanhatt submitted PR review.
cfallin merged PR #5972.
Last updated: Dec 23 2024 at 12:05 UTC