Stream: git-wasmtime

Topic: wasmtime / PR #5972 Cranelift: x64, aarch64, s390x, riscv...

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:03):

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 only I64s 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 a Value and produce an address (gen_load takes a Reg) 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.
-->

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:03):

cfallin requested fitzgen for a review on PR #5972.

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:06):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:06):

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 a Value and produce an address (gen_load takes a Reg) 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.
-->

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:13):

cfallin updated PR #5972 from 64-bit-addresses-are-64-bits to main.

Wasmtime GitHub notifications bot (Mar 09 2023 at 17:14):

cfallin has enabled auto merge for PR #5972.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:06):

cfallin updated PR #5972 from 64-bit-addresses-are-64-bits to main.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:08):

cfallin has disabled auto merge for PR #5972.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:10):

cfallin requested fitzgen for a review on PR #5972.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:18):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:18):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:18):

fitzgen created PR review comment:

Does ty_64 succeed for f64? I guess the CLIF verifier would presumably catch that one, but it may make sense to use a purpose built extractor that is called is_address_type or something like that.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:21):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:21):

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 like is_address_type though; I'll do that.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:26):

cfallin updated PR #5972 from 64-bit-addresses-are-64-bits to main.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:27):

cfallin has enabled auto merge for PR #5972.

Wasmtime GitHub notifications bot (Mar 09 2023 at 18:44):

avanhatt submitted PR review.

Wasmtime GitHub notifications bot (Mar 09 2023 at 19:46):

cfallin merged PR #5972.

Last updated: Nov 22 2024 at 17:03 UTC