x64: Fix possible overflow in Amode::offset (#12949)

* x64: Fix possible overflow in `Amode::offset`

This commit fixes an issue in the x64 backend of Cranelift where the
`Amode::offset` method contained unchecked arithmetic meaning that it
could possibly overflow. This in turn could lead to a miscompile of
loading/storing 128-bit integers where this method is used to generate
an `Amode` that is 8 bytes beyond the based address to load the upper
bits. This miscompile isn't reachable from WebAssembly but is
nonetheless still a good bugfix to have for Cranelift.

The fix here is to switch the `Amode::offset` method to being fallible,
returning `None` on overflow. This then propagates up into ISLE where
the `amode_offset` helper now has a separate case for when the addition
fails, using `lea` to generate a register with an address in it. This
then subsequently also needed fixing for various `Atomic128*` operations
where instead of storing just a single `SyntheticAmode` they now store
two, one for the address of the low bits and one for the address of the
high bits.

* Fix tests

Notably package up all the arguments into a boxed structure for the
atomic128 ops to avoid making `Inst` too large.

* Fix clippy

Author: alexcrichton

cranelift/codegen/src/isa/x64/inst.isle

@@ -236,20 +236,15 @@
        ;;
        ;; For `AtomicRmwOp::Xchg`, use `Atomic128XchgSeq` instead.
        ;;
+       ;; This requires that `mem_high` addresses 8 bytes beyond `mem_low`.
+       ;;
        ;; This instruction sequence has fixed register uses as follows:
-       ;; - %rax (low), %rdx (high)  (written) the old value at `mem`
+       ;; - %rax (low), %rdx (high)  (written) the old value at `mem_low`
        ;; - %rbx (low), %rcx (high)  (written) used as temp registers to hold
        ;;   the replacement value
        ;; - %rflags is written.  Do not assume anything about it after the
        ;;   instruction.
-       (Atomic128RmwSeq (op Atomic128RmwSeqOp)
-                        (mem BoxSyntheticAmode)
-                        (operand_low Gpr)
-                        (operand_high Gpr)
-                        (temp_low WritableGpr)
-                        (temp_high WritableGpr)
-                        (dst_old_low WritableGpr)
-                        (dst_old_high WritableGpr))
+       (Atomic128RmwSeq (args BoxAtomic128RmwSeqArgs))
 
        ;; A synthetic instruction, based on a loop around a native `lock
        ;; cmpxchg16b` instruction.
@@ -258,16 +253,14 @@
        ;; replacement value is the same every time, this instruction doesn't
        ;; require any temporary registers.
        ;;
+       ;; This requires that `mem_high` addresses 8 bytes beyond `mem_low`.
+       ;;
        ;; This instruction sequence has fixed register uses as follows:
-       ;; - %rax (low), %rdx (high)  (written) the old value at `mem`
+       ;; - %rax (low), %rdx (high)  (written) the old value at `mem_low`
        ;; - %rbx (low), %rcx (high)  (read) the replacement value
        ;; - %rflags is written.  Do not assume anything about it after the
        ;;   instruction.
-       (Atomic128XchgSeq (mem SyntheticAmode)
-                         (operand_low Gpr)
-                         (operand_high Gpr)
-                         (dst_old_low WritableGpr)
-                         (dst_old_high WritableGpr))
+       (Atomic128XchgSeq (args BoxAtomic128XchgSeqArgs))
 
        ;; =========================================
        ;; Meta-instructions generating no code.
@@ -338,11 +331,8 @@
 (type BoxCallIndInfo extern (enum))
 (type BoxReturnCallInfo extern (enum))
 (type BoxReturnCallIndInfo extern (enum))
-(type BoxSyntheticAmode extern (enum))
-
-(decl pure box_synthetic_amode (SyntheticAmode) BoxSyntheticAmode)
-(extern constructor box_synthetic_amode box_synthetic_amode)
-(convert SyntheticAmode BoxSyntheticAmode box_synthetic_amode)
+(type BoxAtomic128RmwSeqArgs extern (enum))
+(type BoxAtomic128XchgSeqArgs extern (enum))
 
 ;; Get the `OperandSize` for a given `Type`, rounding smaller types up to 32 bits.
 (decl operand_size_of_type_32_64 (Type) OperandSize)
@@ -352,6 +342,16 @@
 (decl raw_operand_size_of_type (Type) OperandSize)
 (extern constructor raw_operand_size_of_type raw_operand_size_of_type)
 
+(decl atomic128_rmw_seq_args
+  (Atomic128RmwSeqOp SyntheticAmode SyntheticAmode Gpr Gpr WritableGpr WritableGpr WritableGpr WritableGpr)
+  BoxAtomic128RmwSeqArgs)
+(extern constructor atomic128_rmw_seq_args atomic128_rmw_seq_args)
+
+(decl atomic128_xchg_seq_args
+  (SyntheticAmode SyntheticAmode Gpr Gpr WritableGpr WritableGpr)
+  BoxAtomic128XchgSeqArgs)
+(extern constructor atomic128_xchg_seq_args atomic128_xchg_seq_args)
+
 ;; Get the bit width of an `OperandSize`.
 (decl operand_size_bits (OperandSize) u16)
 (rule (operand_size_bits (OperandSize.Size8)) 8)
@@ -588,8 +588,19 @@
 
 ;; Offsetting an Amode. Used when we need to do consecutive
 ;; loads/stores to adjacent addresses.
-(decl amode_offset (SyntheticAmode i32) SyntheticAmode)
-(extern constructor amode_offset amode_offset)
+;;
+;; Note that the input `Amode` might have a static offset that's too large to
+;; add the specified `i32` to so in the case of overflow a new `Amode` is
+;; generated entirely using `lea`
+(decl amode_offset (SyntheticAmode MemFlags i32) SyntheticAmode)
+(rule 1 (amode_offset base _flags offset)
+  (if-let new (amode_try_offset base offset))
+  new)
+(rule 0 (amode_offset base flags offset)
+  (Amode.ImmReg offset (x64_lea $I64 base) flags))
+
+(decl pure partial amode_try_offset (SyntheticAmode i32) SyntheticAmode)
+(extern constructor amode_try_offset amode_try_offset)
 
 ;; Return a zero offset as an `Offset32`.
 (spec (zero_offset) (provide (= result #x00000000)))
@@ -3693,32 +3704,38 @@
             (_ Unit (emit (MInst.AtomicRmwSeq ty op mem input tmp dst))))
         dst))
 
-(decl x64_atomic_128_rmw_seq (AtomicRmwOp SyntheticAmode ValueRegs) ValueRegs)
-(rule (x64_atomic_128_rmw_seq op mem input)
+(decl x64_atomic_128_rmw_seq (AtomicRmwOp SyntheticAmode MemFlags ValueRegs) ValueRegs)
+(rule (x64_atomic_128_rmw_seq op mem_low flags input)
       (let ((dst_low WritableGpr (temp_writable_gpr))
             (dst_high WritableGpr (temp_writable_gpr))
             (tmp_low WritableGpr (temp_writable_gpr))
             (tmp_high WritableGpr (temp_writable_gpr))
             (input_low Gpr (value_regs_get_gpr input 0))
             (input_high Gpr (value_regs_get_gpr input 1))
-            (_ Unit (emit (MInst.Atomic128RmwSeq (atomic_128_rmw_seq_op op) mem input_low input_high tmp_low tmp_high dst_low dst_high))))
+            (mem_high SyntheticAmode (amode_offset mem_low flags 8))
+            (args BoxAtomic128RmwSeqArgs (atomic128_rmw_seq_args (atomic_128_rmw_seq_op op) mem_low mem_high input_low input_high tmp_low tmp_high dst_low dst_high))
+            (_ Unit (emit (MInst.Atomic128RmwSeq args))))
         (value_regs dst_low dst_high)))
 
-(rule 1 (x64_atomic_128_rmw_seq (AtomicRmwOp.Xchg) mem input)
+(rule 1 (x64_atomic_128_rmw_seq (AtomicRmwOp.Xchg) mem_low flags input)
         (let ((dst_low WritableGpr (temp_writable_gpr))
               (dst_high WritableGpr (temp_writable_gpr))
               (input_low Gpr (value_regs_get_gpr input 0))
               (input_high Gpr (value_regs_get_gpr input 1))
-              (_ Unit (emit (MInst.Atomic128XchgSeq mem input_low input_high dst_low dst_high))))
+              (mem_high SyntheticAmode (amode_offset mem_low flags 8))
+              (args BoxAtomic128XchgSeqArgs (atomic128_xchg_seq_args mem_low mem_high input_low input_high dst_low dst_high))
+              (_ Unit (emit (MInst.Atomic128XchgSeq args))))
           (value_regs dst_low dst_high)))
 
-(decl x64_atomic_128_store_seq (SyntheticAmode ValueRegs) SideEffectNoResult)
-(rule (x64_atomic_128_store_seq mem input)
+(decl x64_atomic_128_store_seq (SyntheticAmode MemFlags ValueRegs) SideEffectNoResult)
+(rule (x64_atomic_128_store_seq mem_low flags input)
         (let ((dst_low WritableGpr (temp_writable_gpr))
               (dst_high WritableGpr (temp_writable_gpr))
               (input_low Gpr (value_regs_get_gpr input 0))
-              (input_high Gpr (value_regs_get_gpr input 1)))
-          (SideEffectNoResult.Inst (MInst.Atomic128XchgSeq mem input_low input_high dst_low dst_high))))
+              (input_high Gpr (value_regs_get_gpr input 1))
+              (mem_high SyntheticAmode (amode_offset mem_low flags 8))
+              (args BoxAtomic128XchgSeqArgs (atomic128_xchg_seq_args mem_low mem_high input_low input_high dst_low dst_high)))
+          (SideEffectNoResult.Inst (MInst.Atomic128XchgSeq args))))
 
 
 (type AtomicRmwSeqOp

cranelift/codegen/src/isa/x64/inst/args.rs

@@ -439,14 +439,14 @@ impl Amode {
     }
 
     /// Offset the amode by a fixed offset.
-    pub(crate) fn offset(&self, offset: i32) -> Self {
+    pub(crate) fn offset(&self, offset: i32) -> Option<Self> {
         let mut ret = self.clone();
-        match &mut ret {
-            &mut Amode::ImmReg { ref mut simm32, .. } => *simm32 += offset,
-            &mut Amode::ImmRegRegShift { ref mut simm32, .. } => *simm32 += offset,
+        let simm32 = match &mut ret {
+            Amode::ImmReg { simm32, .. } | Amode::ImmRegRegShift { simm32, .. } => simm32,
             _ => panic!("Cannot offset amode: {self:?}"),
-        }
-        ret
+        };
+        *simm32 = simm32.checked_add(offset)?;
+        Some(ret)
     }
 
     pub(crate) fn aligned(&self) -> bool {
@@ -575,18 +575,18 @@ impl SyntheticAmode {
     }
 
     /// Offset the synthetic amode by a fixed offset.
-    pub(crate) fn offset(&self, offset: i32) -> Self {
+    pub(crate) fn offset(&self, offset: i32) -> Option<Self> {
         let mut ret = self.clone();
         match &mut ret {
-            SyntheticAmode::Real(amode) => *amode = amode.offset(offset),
-            SyntheticAmode::SlotOffset { simm32 } => *simm32 += offset,
+            SyntheticAmode::Real(amode) => *amode = amode.offset(offset)?,
+            SyntheticAmode::SlotOffset { simm32 } => *simm32 = simm32.checked_add(offset)?,
             // `amode_offset` is used only in i128.load/store which
             // takes a synthetic amode from `to_amode`; `to_amode` can
             // only produce Real or SlotOffset amodes, never
             // IncomingArg or ConstantOffset.
             _ => panic!("Cannot offset SyntheticAmode: {self:?}"),
         }
-        ret
+        Some(ret)
     }
 }
 
@@ -1061,3 +1061,34 @@ impl OperandSize {
         self.to_bytes() * 8
     }
 }
+
+pub use crate::isa::x64::lower::isle::generated_code::Atomic128RmwSeqOp;
+
+/// "Package" of the arguments for the instruction `Atomic128RmwSeq` to avoid
+/// making the `Inst` enum massive.
+#[derive(Debug, Clone)]
+#[expect(missing_docs, reason = "self-describing fields")]
+pub struct Atomic128RmwSeqArgs {
+    pub op: Atomic128RmwSeqOp,
+    pub mem_low: SyntheticAmode,
+    pub mem_high: SyntheticAmode,
+    pub operand_low: Gpr,
+    pub operand_high: Gpr,
+    pub temp_low: WritableGpr,
+    pub temp_high: WritableGpr,
+    pub dst_old_low: WritableGpr,
+    pub dst_old_high: WritableGpr,
+}
+
+/// "Package" of the arguments for the instruction `Atomic128XchgSeq` to avoid
+/// making the `Inst` enum massive.
+#[derive(Debug, Clone)]
+#[expect(missing_docs, reason = "self-describing fields")]
+pub struct Atomic128XchgSeqArgs {
+    pub mem_low: SyntheticAmode,
+    pub mem_high: SyntheticAmode,
+    pub operand_low: Gpr,
+    pub operand_high: Gpr,
+    pub dst_old_low: WritableGpr,
+    pub dst_old_high: WritableGpr,
+}

cranelift/codegen/src/isa/x64/inst/emit.rs

@@ -1544,16 +1544,18 @@ pub(crate) fn emit(
             one_way_jmp(sink, CC::NZ, again_label);
         }
 
-        Inst::Atomic128RmwSeq {
-            op,
-            mem,
-            operand_low,
-            operand_high,
-            temp_low,
-            temp_high,
-            dst_old_low,
-            dst_old_high,
-        } => {
+        Inst::Atomic128RmwSeq { args } => {
+            let Atomic128RmwSeqArgs {
+                op,
+                mem_low,
+                mem_high,
+                operand_low,
+                operand_high,
+                temp_low,
+                temp_high,
+                dst_old_low,
+                dst_old_high,
+            } = &**args;
             let operand_low = *operand_low;
             let operand_high = *operand_high;
             let temp_low = *temp_low;
@@ -1564,13 +1566,14 @@ pub(crate) fn emit(
             debug_assert_eq!(temp_high.to_reg(), regs::rcx());
             debug_assert_eq!(dst_old_low.to_reg(), regs::rax());
             debug_assert_eq!(dst_old_high.to_reg(), regs::rdx());
-            let mem = mem.finalize(state.frame_layout(), sink).clone();
+            let mem_low = mem_low.finalize(state.frame_layout(), sink).clone();
+            let mem_high = mem_high.finalize(state.frame_layout(), sink).clone();
 
             let again_label = sink.get_label();
 
             // Load the initial value.
-            asm::inst::movq_rm::new(dst_old_low, mem.clone()).emit(sink, info, state);
-            asm::inst::movq_rm::new(dst_old_high, mem.offset(8)).emit(sink, info, state);
+            asm::inst::movq_rm::new(dst_old_low, mem_low.clone()).emit(sink, info, state);
+            asm::inst::movq_rm::new(dst_old_high, mem_high).emit(sink, info, state);
 
             // again:
             sink.bind_label(again_label, state.ctrl_plane_mut());
@@ -1656,21 +1659,23 @@ pub(crate) fn emit(
                 PairedGpr::from(dst_old_high),
                 temp_low.to_reg(),
                 temp_high.to_reg(),
-                mem,
+                mem_low,
             )
             .emit(sink, info, state);
 
             // jnz again
             one_way_jmp(sink, CC::NZ, again_label);
         }
 
-        Inst::Atomic128XchgSeq {
-            mem,
-            operand_low,
-            operand_high,
-            dst_old_low,
-            dst_old_high,
-        } => {
+        Inst::Atomic128XchgSeq { args } => {
+            let Atomic128XchgSeqArgs {
+                mem_low,
+                mem_high,
+                operand_low,
+                operand_high,
+                dst_old_low,
+                dst_old_high,
+            } = &**args;
             let operand_low = *operand_low;
             let operand_high = *operand_high;
             let dst_old_low = *dst_old_low;
@@ -1679,13 +1684,14 @@ pub(crate) fn emit(
             debug_assert_eq!(operand_high, regs::rcx());
             debug_assert_eq!(dst_old_low.to_reg(), regs::rax());
             debug_assert_eq!(dst_old_high.to_reg(), regs::rdx());
-            let mem = mem.finalize(state.frame_layout(), sink).clone();
+            let mem_low = mem_low.finalize(state.frame_layout(), sink).clone();
+            let mem_high = mem_high.finalize(state.frame_layout(), sink).clone();
 
             let again_label = sink.get_label();
 
             // Load the initial value.
-            asm::inst::movq_rm::new(dst_old_low, mem.clone()).emit(sink, info, state);
-            asm::inst::movq_rm::new(dst_old_high, mem.offset(8)).emit(sink, info, state);
+            asm::inst::movq_rm::new(dst_old_low, mem_low.clone()).emit(sink, info, state);
+            asm::inst::movq_rm::new(dst_old_high, mem_high).emit(sink, info, state);
 
             // again:
             sink.bind_label(again_label, state.ctrl_plane_mut());
@@ -1696,7 +1702,7 @@ pub(crate) fn emit(
                 PairedGpr::from(dst_old_high),
                 operand_low,
                 operand_high,
-                mem,
+                mem_low,
             )
             .emit(sink, info, state);