From c422fdeba276802b2b46d4ac6f3aab68791a5f6f Mon Sep 17 00:00:00 2001 From: Nick Lewycky Date: Mon, 1 Mar 2021 15:00:18 -0800 Subject: [PATCH 1/3] Correct implementation of shift and rotate. The existing implementations worked on x86-64 when instructions were emitted, but relied on UB per the LLVM IR. Add a test which checks the behaviour when the inputs are constants, so that the LLVM IR constant folder can see and exploit the UB. --- lib/compiler-llvm/src/translator/code.rs | 64 +++++-- tests/wast/wasmer/rotate-shift-overflow.wast | 169 +++++++++++++++++++ 2 files changed, 222 insertions(+), 11 deletions(-) create mode 100644 tests/wast/wasmer/rotate-shift-overflow.wast diff --git a/lib/compiler-llvm/src/translator/code.rs b/lib/compiler-llvm/src/translator/code.rs index 594f69e096b..ee012330b70 100644 --- a/lib/compiler-llvm/src/translator/code.rs +++ b/lib/compiler-llvm/src/translator/code.rs @@ -2818,12 +2818,23 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let res = self.builder.build_bitcast(res, self.intrinsics.i128_ty, ""); self.state.push1(res); } - Operator::I32Shl | Operator::I64Shl => { + Operator::I32Shl => { let ((v1, i1), (v2, i2)) = self.state.pop2_extra()?; let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); - // TODO: missing 'and' of v2? + let mask = self.intrinsics.i32_ty.const_int(31u64, false); + let v2 = self.builder.build_and(v2, mask, ""); + let res = self.builder.build_left_shift(v1, v2, ""); + self.state.push1(res); + } + Operator::I64Shl => { + let ((v1, i1), (v2, i2)) = self.state.pop2_extra()?; + let v1 = self.apply_pending_canonicalization(v1, i1); + let v2 = self.apply_pending_canonicalization(v2, i2); + let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); + let mask = self.intrinsics.i32_ty.const_int(63u64, false); + let v2 = self.builder.build_and(v2, mask, ""); let res = self.builder.build_left_shift(v1, v2, ""); self.state.push1(res); } @@ -2888,12 +2899,23 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let res = self.builder.build_bitcast(res, self.intrinsics.i128_ty, ""); self.state.push1(res); } - Operator::I32ShrS | Operator::I64ShrS => { + Operator::I32ShrS => { let ((v1, i1), (v2, i2)) = self.state.pop2_extra()?; let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); - // TODO: check wasm spec, is this missing v2 mod LaneBits? + let mask = self.intrinsics.i32_ty.const_int(31u64, false); + let v2 = self.builder.build_and(v2, mask, ""); + let res = self.builder.build_right_shift(v1, v2, true, ""); + self.state.push1(res); + } + Operator::I64ShrS => { + let ((v1, i1), (v2, i2)) = self.state.pop2_extra()?; + let v1 = self.apply_pending_canonicalization(v1, i1); + let v2 = self.apply_pending_canonicalization(v2, i2); + let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); + let mask = self.intrinsics.i32_ty.const_int(63u64, false); + let v2 = self.builder.build_and(v2, mask, ""); let res = self.builder.build_right_shift(v1, v2, true, ""); self.state.push1(res); } @@ -2958,11 +2980,23 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let res = self.builder.build_bitcast(res, self.intrinsics.i128_ty, ""); self.state.push1(res); } - Operator::I32ShrU | Operator::I64ShrU => { + Operator::I32ShrU => { + let ((v1, i1), (v2, i2)) = self.state.pop2_extra()?; + let v1 = self.apply_pending_canonicalization(v1, i1); + let v2 = self.apply_pending_canonicalization(v2, i2); + let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); + let mask = self.intrinsics.i32_ty.const_int(31u64, false); + let v2 = self.builder.build_and(v2, mask, ""); + let res = self.builder.build_right_shift(v1, v2, false, ""); + self.state.push1(res); + } + Operator::I64ShrU => { let ((v1, i1), (v2, i2)) = self.state.pop2_extra()?; let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); + let mask = self.intrinsics.i32_ty.const_int(63u64, false); + let v2 = self.builder.build_and(v2, mask, ""); let res = self.builder.build_right_shift(v1, v2, false, ""); self.state.push1(res); } @@ -3032,10 +3066,12 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); + let mask = self.intrinsics.i32_ty.const_int(31u64, false); + let v2 = self.builder.build_and(v2, mask, ""); let lhs = self.builder.build_left_shift(v1, v2, ""); let rhs = { - let int_width = self.intrinsics.i32_ty.const_int(32 as u64, false); - let rhs = self.builder.build_int_sub(int_width, v2, ""); + let negv2 = self.builder.build_int_neg(v2, ""); + let rhs = self.builder.build_and(negv2, mask, ""); self.builder.build_right_shift(v1, rhs, false, "") }; let res = self.builder.build_or(lhs, rhs, ""); @@ -3046,10 +3082,12 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); + let mask = self.intrinsics.i32_ty.const_int(63u64, false); + let v2 = self.builder.build_and(v2, mask, ""); let lhs = self.builder.build_left_shift(v1, v2, ""); let rhs = { - let int_width = self.intrinsics.i64_ty.const_int(64 as u64, false); - let rhs = self.builder.build_int_sub(int_width, v2, ""); + let negv2 = self.builder.build_int_neg(v2, ""); + let rhs = self.builder.build_and(negv2, mask, ""); self.builder.build_right_shift(v1, rhs, false, "") }; let res = self.builder.build_or(lhs, rhs, ""); @@ -3060,10 +3098,12 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); + let mask = self.intrinsics.i32_ty.const_int(31u64, false); + let v2 = self.builder.build_and(v2, mask, ""); let lhs = self.builder.build_right_shift(v1, v2, false, ""); let rhs = { - let int_width = self.intrinsics.i32_ty.const_int(32 as u64, false); - let rhs = self.builder.build_int_sub(int_width, v2, ""); + let negv2 = self.builder.build_int_neg(v2, ""); + let rhs = self.builder.build_and(negv2, mask, ""); self.builder.build_left_shift(v1, rhs, "") }; let res = self.builder.build_or(lhs, rhs, ""); @@ -3074,6 +3114,8 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); + let mask = self.intrinsics.i32_ty.const_int(63u64, false); + let v2 = self.builder.build_and(v2, mask, ""); let lhs = self.builder.build_right_shift(v1, v2, false, ""); let rhs = { let int_width = self.intrinsics.i64_ty.const_int(64 as u64, false); diff --git a/tests/wast/wasmer/rotate-shift-overflow.wast b/tests/wast/wasmer/rotate-shift-overflow.wast new file mode 100644 index 00000000000..0eb3b9eaa88 --- /dev/null +++ b/tests/wast/wasmer/rotate-shift-overflow.wast @@ -0,0 +1,169 @@ +;; Test that constant folding which overflows doesn't produce an undefined value. +;; Changing these tests to move the constants to the caller hides the bug. + +(module + ;; shl + (func (export "shl1") (result i32) + i32.const 235 + i32.const 0 + i32.shl + ) + (func (export "shl2") (result i32) + i32.const 235 + i32.const 32 + i32.shl + ) + (func (export "shl3") (result i32) + i32.const 235 + i32.const 100 + i32.shl + ) + (func (export "shl4") (result i32) + i32.const 235 + i32.const -32 + i32.shl + ) + (func (export "shl5") (result i32) + i32.const 235 + i32.const -100 + i32.shl + ) + + ;; shr_u + (func (export "shr_u1") (result i32) + i32.const 235 + i32.const 0 + i32.shr_u + ) + (func (export "shr_u2") (result i32) + i32.const 235 + i32.const 32 + i32.shr_u + ) + (func (export "shr_u3") (result i32) + i32.const 235 + i32.const 100 + i32.shr_u + ) + (func (export "shr_u4") (result i32) + i32.const 235 + i32.const -32 + i32.shr_u + ) + (func (export "shr_u5") (result i32) + i32.const 235 + i32.const -100 + i32.shr_u + ) + + ;; shr_s + (func (export "shr_s1") (result i32) + i32.const 235 + i32.const 0 + i32.shr_s + ) + (func (export "shr_s2") (result i32) + i32.const 235 + i32.const 32 + i32.shr_s + ) + (func (export "shr_s3") (result i32) + i32.const 235 + i32.const 100 + i32.shr_s + ) + (func (export "shr_s4") (result i32) + i32.const 235 + i32.const -32 + i32.shr_s + ) + (func (export "shr_s5") (result i32) + i32.const 235 + i32.const -100 + i32.shr_s + ) + + ;; rotl + (func (export "rotl1") (result i32) + i32.const 235 + i32.const 0 + i32.rotl + ) + (func (export "rotl2") (result i32) + i32.const 235 + i32.const 32 + i32.rotl + ) + (func (export "rotl3") (result i32) + i32.const 235 + i32.const 100 + i32.rotl + ) + (func (export "rotl4") (result i32) + i32.const 235 + i32.const -32 + i32.rotl + ) + (func (export "rotl5") (result i32) + i32.const 235 + i32.const -100 + i32.rotl + ) + + ;; rotr + (func (export "rotr1") (result i32) + i32.const 235 + i32.const 0 + i32.rotr + ) + (func (export "rotr2") (result i32) + i32.const 235 + i32.const 32 + i32.rotr + ) + (func (export "rotr3") (result i32) + i32.const 235 + i32.const 100 + i32.rotr + ) + (func (export "rotr4") (result i32) + i32.const 235 + i32.const -32 + i32.rotr + ) + (func (export "rotr5") (result i32) + i32.const 235 + i32.const -100 + i32.rotr + ) +) + +(assert_return (invoke "shl1") (i32.const 235)) +(assert_return (invoke "shl2") (i32.const 235)) +(assert_return (invoke "shl3") (i32.const 3760)) +(assert_return (invoke "shl4") (i32.const 235)) +(assert_return (invoke "shl5") (i32.const -1342177280)) + +(assert_return (invoke "shr_u1") (i32.const 235)) +(assert_return (invoke "shr_u2") (i32.const 235)) +(assert_return (invoke "shr_u3") (i32.const 14)) +(assert_return (invoke "shr_u4") (i32.const 235)) +(assert_return (invoke "shr_u5") (i32.const 0)) + +(assert_return (invoke "shr_s1") (i32.const 235)) +(assert_return (invoke "shr_s2") (i32.const 235)) +(assert_return (invoke "shr_s3") (i32.const 14)) +(assert_return (invoke "shr_s4") (i32.const 235)) +(assert_return (invoke "shr_s5") (i32.const 0)) + +(assert_return (invoke "rotl1") (i32.const 235)) +(assert_return (invoke "rotl2") (i32.const 235)) +(assert_return (invoke "rotl3") (i32.const 3760)) +(assert_return (invoke "rotl4") (i32.const 235)) +(assert_return (invoke "rotl5") (i32.const -1342177266)) + +(assert_return (invoke "rotr1") (i32.const 235)) +(assert_return (invoke "rotr2") (i32.const 235)) +(assert_return (invoke "rotr3") (i32.const -1342177266)) +(assert_return (invoke "rotr4") (i32.const 235)) +(assert_return (invoke "rotr5") (i32.const 3760)) From 69b2a5a9d4ef76c561eb031ac1e4e7d801a7d5d5 Mon Sep 17 00:00:00 2001 From: Nick Lewycky Date: Mon, 1 Mar 2021 15:03:20 -0800 Subject: [PATCH 2/3] Improve wording of comment. --- tests/wast/wasmer/rotate-shift-overflow.wast | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/wast/wasmer/rotate-shift-overflow.wast b/tests/wast/wasmer/rotate-shift-overflow.wast index 0eb3b9eaa88..fa2efee2459 100644 --- a/tests/wast/wasmer/rotate-shift-overflow.wast +++ b/tests/wast/wasmer/rotate-shift-overflow.wast @@ -1,5 +1,6 @@ -;; Test that constant folding which overflows doesn't produce an undefined value. -;; Changing these tests to move the constants to the caller hides the bug. +;; Test that constant folding an operation that overflows doesn't produce an +;; undefined value. Changing these tests to move the constants to the +;; assert_return line hides the bug. (module ;; shl From cc27bb2216a09fd654e2f460dae0024fd59f9242 Mon Sep 17 00:00:00 2001 From: Nick Lewycky Date: Mon, 1 Mar 2021 15:50:47 -0800 Subject: [PATCH 3/3] Fix some verifier errors in the 64-bit computations. --- lib/compiler-llvm/src/translator/code.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/compiler-llvm/src/translator/code.rs b/lib/compiler-llvm/src/translator/code.rs index ee012330b70..6a668c347ee 100644 --- a/lib/compiler-llvm/src/translator/code.rs +++ b/lib/compiler-llvm/src/translator/code.rs @@ -2833,7 +2833,7 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); - let mask = self.intrinsics.i32_ty.const_int(63u64, false); + let mask = self.intrinsics.i64_ty.const_int(63u64, false); let v2 = self.builder.build_and(v2, mask, ""); let res = self.builder.build_left_shift(v1, v2, ""); self.state.push1(res); @@ -2914,7 +2914,7 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); - let mask = self.intrinsics.i32_ty.const_int(63u64, false); + let mask = self.intrinsics.i64_ty.const_int(63u64, false); let v2 = self.builder.build_and(v2, mask, ""); let res = self.builder.build_right_shift(v1, v2, true, ""); self.state.push1(res); @@ -2995,7 +2995,7 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); - let mask = self.intrinsics.i32_ty.const_int(63u64, false); + let mask = self.intrinsics.i64_ty.const_int(63u64, false); let v2 = self.builder.build_and(v2, mask, ""); let res = self.builder.build_right_shift(v1, v2, false, ""); self.state.push1(res); @@ -3082,7 +3082,7 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); - let mask = self.intrinsics.i32_ty.const_int(63u64, false); + let mask = self.intrinsics.i64_ty.const_int(63u64, false); let v2 = self.builder.build_and(v2, mask, ""); let lhs = self.builder.build_left_shift(v1, v2, ""); let rhs = { @@ -3114,7 +3114,7 @@ impl<'ctx, 'a> LLVMFunctionCodeGenerator<'ctx, 'a> { let v1 = self.apply_pending_canonicalization(v1, i1); let v2 = self.apply_pending_canonicalization(v2, i2); let (v1, v2) = (v1.into_int_value(), v2.into_int_value()); - let mask = self.intrinsics.i32_ty.const_int(63u64, false); + let mask = self.intrinsics.i64_ty.const_int(63u64, false); let v2 = self.builder.build_and(v2, mask, ""); let lhs = self.builder.build_right_shift(v1, v2, false, ""); let rhs = {