From 3e80ed720a6e25e7fcad2b801ca388aa909cc6ff Mon Sep 17 00:00:00 2001 From: Quentin Monnet Date: Fri, 15 Mar 2024 11:43:21 +0000 Subject: [PATCH] src/interpreter: Remove/explain masks for 32-bit shift operations We recently fixed the 64-bit arithmetic shift-right operations by adding a mask to the number of bits (as immediates or from the source register) by which to shift, as per the eBPF ISA specification. The 32-bit operations must use the same mask, but .wrapping_shr() already takes care of it for us. Let's add a comment to make it explicit. As it turns out, masking is just as well unnecessary for the non-arithmetic left-right shift operations that we tried to fix recently. Let's also remove the mask there. Signed-off-by: Quentin Monnet --- src/interpreter.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/interpreter.rs b/src/interpreter.rs index f5394cc3..31c95a57 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -37,7 +37,6 @@ fn check_mem(addr: u64, len: usize, access_type: &str, insn_ptr: usize, #[allow(cyclomatic_complexity)] pub fn execute_program(prog_: Option<&[u8]>, mem: &[u8], mbuff: &[u8], helpers: &HashMap) -> Result { const U32MAX: u64 = u32::MAX as u64; - const SHIFT_MASK_32: u32 = 0x1f; const SHIFT_MASK_64: u64 = 0x3f; let prog = match prog_ { @@ -226,10 +225,12 @@ pub fn execute_program(prog_: Option<&[u8]>, mem: &[u8], mbuff: &[u8], helpers: ebpf::OR32_REG => reg[_dst] = (reg[_dst] as u32 | reg[_src] as u32) as u64, ebpf::AND32_IMM => reg[_dst] = (reg[_dst] as u32 & insn.imm as u32) as u64, ebpf::AND32_REG => reg[_dst] = (reg[_dst] as u32 & reg[_src] as u32) as u64, - ebpf::LSH32_IMM => reg[_dst] = (reg[_dst] as u32).wrapping_shl(insn.imm as u32 & SHIFT_MASK_32) as u64, - ebpf::LSH32_REG => reg[_dst] = (reg[_dst] as u32).wrapping_shl(reg[_src] as u32 & SHIFT_MASK_32) as u64, - ebpf::RSH32_IMM => reg[_dst] = (reg[_dst] as u32).wrapping_shr(insn.imm as u32 & SHIFT_MASK_32) as u64, - ebpf::RSH32_REG => reg[_dst] = (reg[_dst] as u32).wrapping_shr(reg[_src] as u32 & SHIFT_MASK_32) as u64, + // As for the 64-bit version, we should mask the number of bits to shift with + // 0x1f, but .wrappping_shr() already takes care of it for us. + ebpf::LSH32_IMM => reg[_dst] = (reg[_dst] as u32).wrapping_shl(insn.imm as u32) as u64, + ebpf::LSH32_REG => reg[_dst] = (reg[_dst] as u32).wrapping_shl(reg[_src] as u32) as u64, + ebpf::RSH32_IMM => reg[_dst] = (reg[_dst] as u32).wrapping_shr(insn.imm as u32) as u64, + ebpf::RSH32_REG => reg[_dst] = (reg[_dst] as u32).wrapping_shr(reg[_src] as u32) as u64, ebpf::NEG32 => { reg[_dst] = (reg[_dst] as i32).wrapping_neg() as u64; reg[_dst] &= U32MAX; }, ebpf::MOD32_IMM if insn.imm as u32 == 0 => (), ebpf::MOD32_IMM => reg[_dst] = (reg[_dst] as u32 % insn.imm as u32) as u64, @@ -239,6 +240,8 @@ pub fn execute_program(prog_: Option<&[u8]>, mem: &[u8], mbuff: &[u8], helpers: ebpf::XOR32_REG => reg[_dst] = (reg[_dst] as u32 ^ reg[_src] as u32) as u64, ebpf::MOV32_IMM => reg[_dst] = insn.imm as u32 as u64, ebpf::MOV32_REG => reg[_dst] = (reg[_src] as u32) as u64, + // As for the 64-bit version, we should mask the number of bits to shift with + // 0x1f, but .wrappping_shr() already takes care of it for us. ebpf::ARSH32_IMM => { reg[_dst] = (reg[_dst] as i32).wrapping_shr(insn.imm as u32) as u64; reg[_dst] &= U32MAX; }, ebpf::ARSH32_REG => { reg[_dst] = (reg[_dst] as i32).wrapping_shr(reg[_src] as u32) as u64; reg[_dst] &= U32MAX; }, ebpf::LE => {