From 7f3500a17200c58c2bb2456fb6c3e0b96fa6a032 Mon Sep 17 00:00:00 2001 From: Chris Fallin Date: Thu, 9 Mar 2023 11:08:16 -0800 Subject: [PATCH] Cranelift: x64, aarch64, s390x, riscv64: ensure addresses are I64s. (#5972) * Cranelift: x64, aarch64, s390x, riscv64: ensure addresses are I64s. @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. 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. * Allow R64 as well as I64 types. * Add an explicit extractor to match 64-bit address types. --- .../codegen/src/isa/aarch64/lower/isle.rs | 4 +++ cranelift/codegen/src/isa/riscv64/lower.isle | 26 +++++++++---------- cranelift/codegen/src/isa/s390x/inst.isle | 8 +++--- cranelift/codegen/src/isa/x64/inst.isle | 2 +- cranelift/codegen/src/isle_prelude.rs | 8 ++++++ cranelift/codegen/src/prelude.isle | 4 +++ 6 files changed, 34 insertions(+), 18 deletions(-) diff --git a/cranelift/codegen/src/isa/aarch64/lower/isle.rs b/cranelift/codegen/src/isa/aarch64/lower/isle.rs index 69796c4bc6..57e187142c 100644 --- a/cranelift/codegen/src/isa/aarch64/lower/isle.rs +++ b/cranelift/codegen/src/isa/aarch64/lower/isle.rs @@ -513,10 +513,14 @@ impl Context for IsleContext<'_, '_, MInst, AArch64Backend> { } fn amode(&mut self, ty: Type, addr: Value, offset: u32) -> AMode { + let addr_ty = self.value_type(addr); + assert!(addr_ty == I64 || addr_ty == R64); lower_address(self.lower_ctx, ty, addr, offset as i32) } fn pair_amode(&mut self, addr: Value, offset: u32) -> PairAMode { + let addr_ty = self.value_type(addr); + assert!(addr_ty == I64 || addr_ty == R64); lower_pair_address(self.lower_ctx, addr, offset as i32) } diff --git a/cranelift/codegen/src/isa/riscv64/lower.isle b/cranelift/codegen/src/isa/riscv64/lower.isle index b08d6558f6..0a15865e72 100644 --- a/cranelift/codegen/src/isa/riscv64/lower.isle +++ b/cranelift/codegen/src/isa/riscv64/lower.isle @@ -697,63 +697,63 @@ ;;;;; Rules for `uload8`;;;;;;;;; (rule - (lower (uload8 flags p offset)) + (lower (uload8 flags p @ (value_type (ty_addr64 _)) offset)) (gen_load p offset (int_load_op $false 8) flags $I64)) ;;;;; Rules for `sload8`;;;;;;;;; (rule - (lower (sload8 flags p offset)) + (lower (sload8 flags p @ (value_type (ty_addr64 _)) offset)) (gen_load p offset (int_load_op $true 8) flags $I64)) ;;;;; Rules for `uload16`;;;;;;;;; (rule - (lower (uload16 flags p offset)) + (lower (uload16 flags p @ (value_type (ty_addr64 _)) offset)) (gen_load p offset (int_load_op $false 16) flags $I64)) ;;;;; Rules for `iload16`;;;;;;;;; (rule - (lower (sload16 flags p offset)) + (lower (sload16 flags p @ (value_type (ty_addr64 _)) offset)) (gen_load p offset (int_load_op $true 16) flags $I64)) ;;;;; Rules for `uload32`;;;;;;;;; (rule - (lower (uload32 flags p offset)) + (lower (uload32 flags p @ (value_type (ty_addr64 _)) offset)) (gen_load p offset (int_load_op $false 32) flags $I64)) ;;;;; Rules for `iload16`;;;;;;;;; (rule - (lower (sload32 flags p offset)) + (lower (sload32 flags p @ (value_type (ty_addr64 _)) offset)) (gen_load p offset (int_load_op $true 32) flags $I64)) (rule - (lower (has_type ty (load flags p offset))) + (lower (has_type ty (load flags p @ (value_type (ty_addr64 _)) offset))) (gen_load p offset (load_op ty) flags ty) ) ;;;; for I128 (rule 1 - (lower (has_type $I128 (load flags p offset))) + (lower (has_type $I128 (load flags p @ (value_type (ty_addr64 _)) offset))) (gen_load_128 p offset flags)) ;;;;; Rules for `istore8`;;;;;;;;; (rule - (lower (istore8 flags x p offset)) + (lower (istore8 flags x p @ (value_type (ty_addr64 _)) offset)) (gen_store p offset (StoreOP.Sb) flags x)) ;;;;; Rules for `istore16`;;;;;;;;; (rule - (lower (istore16 flags x p offset)) + (lower (istore16 flags x p @ (value_type (ty_addr64 _)) offset)) (gen_store p offset (StoreOP.Sh) flags x)) ;;;;; Rules for `istore32`;;;;;;;;; (rule - (lower (istore32 flags x p offset)) + (lower (istore32 flags x p @ (value_type (ty_addr64 _)) offset)) (gen_store p offset (StoreOP.Sw) flags x)) ;;;;; Rules for `store`;;;;;;;;; (rule - (lower (store flags x @ (value_type ty) p offset)) + (lower (store flags x @ (value_type ty) p @ (value_type (ty_addr64 _)) offset)) (gen_store p offset (store_op ty) flags x)) ;;; special for I128 (rule 1 - (lower (store flags x @ (value_type $I128 ) p offset)) + (lower (store flags x @ (value_type $I128 ) p @ (value_type (ty_addr64 _)) offset)) (gen_store_128 p offset flags x)) (decl gen_icmp (IntCC ValueRegs ValueRegs Type) Reg) diff --git a/cranelift/codegen/src/isa/s390x/inst.isle b/cranelift/codegen/src/isa/s390x/inst.isle index 815513f046..ffc5f7350f 100644 --- a/cranelift/codegen/src/isa/s390x/inst.isle +++ b/cranelift/codegen/src/isa/s390x/inst.isle @@ -1811,10 +1811,10 @@ (decl lower_address (MemFlags Value Offset32) MemArg) -(rule (lower_address flags addr (i64_from_offset offset)) +(rule (lower_address flags addr @ (value_type (ty_addr64 _)) (i64_from_offset offset)) (memarg_reg_plus_off addr offset 0 flags)) -(rule 1 (lower_address flags (iadd x y) (i64_from_offset 0)) +(rule 1 (lower_address flags (has_type (ty_addr64 _) (iadd x y)) (i64_from_offset 0)) (memarg_reg_plus_reg x y 0 flags)) (rule 1 (lower_address flags @@ -1828,10 +1828,10 @@ (decl lower_address_bias (MemFlags Value Offset32 u8) MemArg) -(rule (lower_address_bias flags addr (i64_from_offset offset) bias) +(rule (lower_address_bias flags addr @ (value_type $I64) (i64_from_offset offset) bias) (memarg_reg_plus_off addr offset bias flags)) -(rule 1 (lower_address_bias flags (iadd x y) (i64_from_offset 0) bias) +(rule 1 (lower_address_bias flags (has_type $I64 (iadd x y)) (i64_from_offset 0) bias) (memarg_reg_plus_reg x y bias flags)) diff --git a/cranelift/codegen/src/isa/x64/inst.isle b/cranelift/codegen/src/isa/x64/inst.isle index 5293d7071a..260796cc5e 100644 --- a/cranelift/codegen/src/isa/x64/inst.isle +++ b/cranelift/codegen/src/isa/x64/inst.isle @@ -1114,7 +1114,7 @@ (Amode.ImmRegRegShift off sum index shift flags))) ;; Finally, define the toplevel `to_amode`. -(rule (to_amode flags base offset) +(rule (to_amode flags base @ (value_type (ty_addr64 _)) offset) (amode_finalize (amode_add (amode_initial flags offset) base))) ;; If an amode has no registers at all and only offsets (a constant diff --git a/cranelift/codegen/src/isle_prelude.rs b/cranelift/codegen/src/isle_prelude.rs index 435955fa21..aa3714f141 100644 --- a/cranelift/codegen/src/isle_prelude.rs +++ b/cranelift/codegen/src/isle_prelude.rs @@ -412,6 +412,14 @@ macro_rules! isle_common_prelude_methods { } } + #[inline] + fn ty_addr64(&mut self, ty: Type) -> Option { + match ty { + I64 | R64 => Some(ty), + _ => None, + } + } + #[inline] fn u64_from_imm64(&mut self, imm: Imm64) -> u64 { imm.bits() as u64 diff --git a/cranelift/codegen/src/prelude.isle b/cranelift/codegen/src/prelude.isle index 89fc8e9813..fb22e29c0b 100644 --- a/cranelift/codegen/src/prelude.isle +++ b/cranelift/codegen/src/prelude.isle @@ -347,6 +347,10 @@ (decl ty_vec128_int (Type) Type) (extern extractor ty_vec128_int ty_vec128_int) +;; An extractor that only matches types that can be a 64-bit address. +(decl ty_addr64 (Type) Type) +(extern extractor ty_addr64 ty_addr64) + ;; A pure constructor that matches everything except vectors with size 32X2. (decl pure partial not_vec32x2 (Type) Type) (extern constructor not_vec32x2 not_vec32x2)