From b880c8ce837f2b936d3da760c9fad500c56ae39e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Jourde?= Date: Fri, 1 Sep 2023 01:08:07 +0200 Subject: [PATCH] cranelift: Validate `iconst` ranges (#6850) * cranelift: Validate `iconst` ranges Add the following checks: `iconst.i8` immediate must be within 0 .. 2^8-1 `iconst.i16` immediate must be within 0 .. 2^16-1 `iconst.i32` immediate must be within 0 .. 2^32-1 Resolves #3059 * cranelift: Parse `iconst` according to its type Modifies the parser for textual CLIF so that V in `iconst.T V` is parsed according to T. Before this commit, something like `iconst.i32 0xffff_ffff_ffff` was valid because all `iconst` were parsed the same as an `iconst.i64`. Now the above example will throw an error. Also, a negative immediate as in `iconst.iN -X` is now converted to `2^N - X`. This commit also fixes some broken tests. * cranelift: Update tests to match new CLIF parser --- cranelift/codegen/src/legalizer/mod.rs | 14 ++- cranelift/codegen/src/verifier/mod.rs | 104 +++++++++++++++++- .../filetests/filetests/egraph/extends.clif | 2 +- .../filetests/isa/s390x/constants.clif | 8 +- .../isa/x64/simd-lane-access-compile.clif | 4 +- .../filetests/filetests/parser/tiny.clif | 2 +- cranelift/reader/src/parser.rs | 24 +++- cranelift/tests/bugpoint_test.clif | 2 +- cranelift/wasm/src/code_translator.rs | 4 +- cranelift/wasm/src/environ/dummy.rs | 12 +- 10 files changed, 153 insertions(+), 23 deletions(-) diff --git a/cranelift/codegen/src/legalizer/mod.rs b/cranelift/codegen/src/legalizer/mod.rs index 3b09ed1fc733..e186b153091d 100644 --- a/cranelift/codegen/src/legalizer/mod.rs +++ b/cranelift/codegen/src/legalizer/mod.rs @@ -16,7 +16,7 @@ use crate::cursor::{Cursor, FuncCursor}; use crate::flowgraph::ControlFlowGraph; use crate::ir::immediates::Imm64; -use crate::ir::types::{I128, I64}; +use crate::ir::types::{self, I128, I64}; use crate::ir::{self, InstBuilder, InstructionData, MemFlags, Value}; use crate::isa::TargetIsa; use crate::trace; @@ -38,7 +38,17 @@ fn imm_const(pos: &mut FuncCursor, arg: Value, imm: Imm64, is_signed: bool) -> V let imm = pos.ins().iconst(I64, imm); pos.ins().uextend(I128, imm) } - _ => pos.ins().iconst(ty.lane_type(), imm), + _ => { + let bits = imm.bits(); + let unsigned = match ty.lane_type() { + types::I8 => bits as u8 as i64, + types::I16 => bits as u16 as i64, + types::I32 => bits as u32 as i64, + types::I64 => bits, + _ => unreachable!(), + }; + pos.ins().iconst(ty.lane_type(), unsigned) + } } } diff --git a/cranelift/codegen/src/verifier/mod.rs b/cranelift/codegen/src/verifier/mod.rs index 0927fe9f06f6..5eaecaa190fa 100644 --- a/cranelift/codegen/src/verifier/mod.rs +++ b/cranelift/codegen/src/verifier/mod.rs @@ -1659,6 +1659,39 @@ impl<'a> Verifier<'a> { } } + fn iconst_bounds(&self, inst: Inst, errors: &mut VerifierErrors) -> VerifierStepResult<()> { + use crate::ir::instructions::InstructionData::UnaryImm; + + let inst_data = &self.func.dfg.insts[inst]; + if let UnaryImm { + opcode: Opcode::Iconst, + imm, + } = inst_data + { + let ctrl_typevar = self.func.dfg.ctrl_typevar(inst); + let bounds_mask = match ctrl_typevar { + types::I8 => u8::MAX.into(), + types::I16 => u16::MAX.into(), + types::I32 => u32::MAX.into(), + types::I64 => u64::MAX, + _ => unreachable!(), + }; + + let value = imm.bits() as u64; + if value & bounds_mask != value { + errors.fatal(( + inst, + self.context(inst), + "constant immediate is out of bounds", + )) + } else { + Ok(()) + } + } else { + Ok(()) + } + } + fn typecheck_function_signature(&self, errors: &mut VerifierErrors) -> VerifierStepResult<()> { let params = self .func @@ -1735,6 +1768,7 @@ impl<'a> Verifier<'a> { self.instruction_integrity(inst, errors)?; self.typecheck(inst, errors)?; self.immediate_constraints(inst, errors)?; + self.iconst_bounds(inst, errors)?; } self.encodable_as_bb(block, errors)?; @@ -1755,7 +1789,7 @@ impl<'a> Verifier<'a> { mod tests { use super::{Verifier, VerifierError, VerifierErrors}; use crate::ir::instructions::{InstructionData, Opcode}; - use crate::ir::{types, AbiParam, Function}; + use crate::ir::{types, AbiParam, Function, Type}; use crate::settings; macro_rules! assert_err_with_msg { @@ -1812,6 +1846,74 @@ mod tests { assert_err_with_msg!(errors, "instruction format"); } + fn test_iconst_bounds(immediate: i64, ctrl_typevar: Type) -> VerifierErrors { + let mut func = Function::new(); + let block0 = func.dfg.make_block(); + func.layout.append_block(block0); + + let test_inst = func.dfg.make_inst(InstructionData::UnaryImm { + opcode: Opcode::Iconst, + imm: immediate.into(), + }); + + let end_inst = func.dfg.make_inst(InstructionData::MultiAry { + opcode: Opcode::Return, + args: Default::default(), + }); + + func.dfg.append_result(test_inst, ctrl_typevar); + func.layout.append_inst(test_inst, block0); + func.layout.append_inst(end_inst, block0); + + let flags = &settings::Flags::new(settings::builder()); + let verifier = Verifier::new(&func, flags.into()); + let mut errors = VerifierErrors::default(); + + let _ = verifier.run(&mut errors); + errors + } + + fn test_iconst_bounds_err(immediate: i64, ctrl_typevar: Type) { + assert_err_with_msg!( + test_iconst_bounds(immediate, ctrl_typevar), + "constant immediate is out of bounds" + ); + } + + fn test_iconst_bounds_ok(immediate: i64, ctrl_typevar: Type) { + assert!(test_iconst_bounds(immediate, ctrl_typevar).is_empty()); + } + + #[test] + fn negative_iconst_8() { + test_iconst_bounds_err(-10, types::I8); + } + + #[test] + fn negative_iconst_32() { + test_iconst_bounds_err(-1, types::I32); + } + + #[test] + fn large_iconst_8() { + test_iconst_bounds_err(1 + u8::MAX as i64, types::I8); + } + + #[test] + fn large_iconst_16() { + test_iconst_bounds_err(10 + u16::MAX as i64, types::I16); + } + + #[test] + fn valid_iconst_8() { + test_iconst_bounds_ok(10, types::I8); + } + + #[test] + fn valid_iconst_32() { + test_iconst_bounds_ok(u32::MAX as i64, types::I32); + } + #[test] fn test_function_invalid_param() { let mut func = Function::new(); diff --git a/cranelift/filetests/filetests/egraph/extends.clif b/cranelift/filetests/filetests/egraph/extends.clif index 03454ca6d0d1..7ebb2642f572 100644 --- a/cranelift/filetests/filetests/egraph/extends.clif +++ b/cranelift/filetests/filetests/egraph/extends.clif @@ -4,7 +4,7 @@ target x86_64 function %f1() -> i64 { block0: - v0 = iconst.i32 0xffff_ffff_9876_5432 + v0 = iconst.i32 0x9876_5432 v1 = uextend.i64 v0 return v1 ; check: v2 = iconst.i64 0x9876_5432 diff --git a/cranelift/filetests/filetests/isa/s390x/constants.clif b/cranelift/filetests/filetests/isa/s390x/constants.clif index 4ae8d7f78567..24a4c1f3552c 100644 --- a/cranelift/filetests/filetests/isa/s390x/constants.clif +++ b/cranelift/filetests/filetests/isa/s390x/constants.clif @@ -9,12 +9,12 @@ block0: ; VCode: ; block0: -; lhi %r2, -1 +; lhi %r2, 255 ; br %r14 ; ; Disassembled: ; block0: ; offset 0x0 -; lhi %r2, -1 +; lhi %r2, 0xff ; br %r14 function %f() -> i16 { @@ -189,11 +189,11 @@ block0: ; VCode: ; block0: -; lhi %r2, -1 +; iilf %r2, 4294967295 ; br %r14 ; ; Disassembled: ; block0: ; offset 0x0 -; lhi %r2, -1 +; iilf %r2, 0xffffffff ; br %r14 diff --git a/cranelift/filetests/filetests/isa/x64/simd-lane-access-compile.clif b/cranelift/filetests/filetests/isa/x64/simd-lane-access-compile.clif index 27f212ff2c7d..3331c776586c 100644 --- a/cranelift/filetests/filetests/isa/x64/simd-lane-access-compile.clif +++ b/cranelift/filetests/filetests/isa/x64/simd-lane-access-compile.clif @@ -193,7 +193,7 @@ block0: ; pushq %rbp ; movq %rsp, %rbp ; block0: -; movl $-1, %ecx +; movl $65535, %ecx ; movd %ecx, %xmm1 ; pshuflw $0, %xmm1, %xmm3 ; pshufd $0, %xmm3, %xmm0 @@ -206,7 +206,7 @@ block0: ; pushq %rbp ; movq %rsp, %rbp ; block1: ; offset 0x4 -; movl $0xffffffff, %ecx +; movl $0xffff, %ecx ; movd %ecx, %xmm1 ; pshuflw $0, %xmm1, %xmm3 ; pshufd $0, %xmm3, %xmm0 diff --git a/cranelift/filetests/filetests/parser/tiny.clif b/cranelift/filetests/filetests/parser/tiny.clif index 9b73213ab1cf..90a8f28aa309 100644 --- a/cranelift/filetests/filetests/parser/tiny.clif +++ b/cranelift/filetests/filetests/parser/tiny.clif @@ -36,7 +36,7 @@ block0: } ; sameln: function %bvalues() fast { ; nextln: block0: -; nextln: v0 = iconst.i32 -1 +; nextln: v0 = iconst.i32 0xffff_ffff ; nextln: v1 = iconst.i8 0 ; nextln: v2 = sextend.i32 v1 ; nextln: v3 = bxor v0, v2 diff --git a/cranelift/reader/src/parser.rs b/cranelift/reader/src/parser.rs index 7beaedb8e95e..28b6ae18600c 100644 --- a/cranelift/reader/src/parser.rs +++ b/cranelift/reader/src/parser.rs @@ -12,6 +12,7 @@ use cranelift_codegen::entity::{EntityRef, PrimaryMap}; use cranelift_codegen::ir::entities::{AnyEntity, DynamicType}; use cranelift_codegen::ir::immediates::{Ieee32, Ieee64, Imm64, Offset32, Uimm32, Uimm64}; use cranelift_codegen::ir::instructions::{InstructionData, InstructionFormat, VariableArgs}; +use cranelift_codegen::ir::types; use cranelift_codegen::ir::types::INVALID; use cranelift_codegen::ir::types::*; use cranelift_codegen::ir::{self, UserExternalNameRef}; @@ -2456,10 +2457,25 @@ impl<'a> Parser<'a> { opcode, arg: self.match_value("expected SSA value operand")?, }, - InstructionFormat::UnaryImm => InstructionData::UnaryImm { - opcode, - imm: self.match_imm64("expected immediate integer operand")?, - }, + InstructionFormat::UnaryImm => { + let msg = |bits| format!("expected immediate {bits}-bit integer operand"); + let unsigned = match explicit_control_type { + Some(types::I8) => self.match_imm8(&msg(8))? as u8 as i64, + Some(types::I16) => self.match_imm16(&msg(16))? as u16 as i64, + Some(types::I32) => self.match_imm32(&msg(32))? as u32 as i64, + Some(types::I64) => self.match_imm64(&msg(64))?.bits(), + _ => { + return err!( + self.loc, + "expected one of the following type: i8, i16, i32 or i64" + ) + } + }; + InstructionData::UnaryImm { + opcode, + imm: Imm64::new(unsigned), + } + } InstructionFormat::UnaryIeee32 => InstructionData::UnaryIeee32 { opcode, imm: self.match_ieee32("expected immediate 32-bit float operand")?, diff --git a/cranelift/tests/bugpoint_test.clif b/cranelift/tests/bugpoint_test.clif index f52264543bc5..47bcb734adba 100644 --- a/cranelift/tests/bugpoint_test.clif +++ b/cranelift/tests/bugpoint_test.clif @@ -910,7 +910,7 @@ block51: v428 = iadd_imm.i64 v28, 8 v429 = load.i16 v428 v435 -> v429 - v430 = iconst.i16 0xffff_ffff_ffff_8000 + v430 = iconst.i16 0x8000 v431 = icmp eq v429, v430 v433 = uextend.i32 v431 brif v433, block154, block52 diff --git a/cranelift/wasm/src/code_translator.rs b/cranelift/wasm/src/code_translator.rs index ac9b28b893e3..d683a420d667 100644 --- a/cranelift/wasm/src/code_translator.rs +++ b/cranelift/wasm/src/code_translator.rs @@ -928,7 +928,9 @@ pub fn translate_operator( translate_store(memarg, ir::Opcode::Store, builder, state, environ)?; } /****************************** Nullary Operators ************************************/ - Operator::I32Const { value } => state.push1(builder.ins().iconst(I32, i64::from(*value))), + Operator::I32Const { value } => { + state.push1(builder.ins().iconst(I32, *value as u32 as i64)) + } Operator::I64Const { value } => state.push1(builder.ins().iconst(I64, *value)), Operator::F32Const { value } => { state.push1(builder.ins().f32const(f32_translation(*value))); diff --git a/cranelift/wasm/src/environ/dummy.rs b/cranelift/wasm/src/environ/dummy.rs index 86622c98c41e..95f9fecece62 100644 --- a/cranelift/wasm/src/environ/dummy.rs +++ b/cranelift/wasm/src/environ/dummy.rs @@ -509,7 +509,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ _heap: Heap, _val: ir::Value, ) -> WasmResult { - Ok(pos.ins().iconst(I32, -1)) + Ok(pos.ins().iconst(I32, -1i32 as u32 as i64)) } fn translate_memory_size( @@ -518,7 +518,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ _index: MemoryIndex, _heap: Heap, ) -> WasmResult { - Ok(pos.ins().iconst(I32, -1)) + Ok(pos.ins().iconst(I32, -1i32 as u32 as i64)) } fn translate_memory_copy( @@ -570,7 +570,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ _index: TableIndex, _table: ir::Table, ) -> WasmResult { - Ok(pos.ins().iconst(I32, -1)) + Ok(pos.ins().iconst(I32, -1i32 as u32 as i64)) } fn translate_table_grow( @@ -581,7 +581,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ _delta: ir::Value, _init_value: ir::Value, ) -> WasmResult { - Ok(pos.ins().iconst(I32, -1)) + Ok(pos.ins().iconst(I32, -1i32 as u32 as i64)) } fn translate_table_get( @@ -660,7 +660,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ mut pos: FuncCursor, _global_index: GlobalIndex, ) -> WasmResult { - Ok(pos.ins().iconst(I32, -1)) + Ok(pos.ins().iconst(I32, -1i32 as u32 as i64)) } fn translate_custom_global_set( @@ -681,7 +681,7 @@ impl<'dummy_environment> FuncEnvironment for DummyFuncEnvironment<'dummy_environ _expected: ir::Value, _timeout: ir::Value, ) -> WasmResult { - Ok(pos.ins().iconst(I32, -1)) + Ok(pos.ins().iconst(I32, -1i32 as u32 as i64)) } fn translate_atomic_notify(