From ae90876b6f884ce440ae9b05485b6f16d38b3dfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Tue, 10 Oct 2023 23:45:25 +0200 Subject: [PATCH] Removes ELF_INSN_DUMP_OFFSET. (#541) --- src/ebpf.rs | 4 ---- src/elf.rs | 15 ++++---------- src/static_analysis.rs | 2 +- src/verifier.rs | 34 +++++++++++-------------------- tests/execution.rs | 2 +- tests/verifier.rs | 45 +++++++++++++++++++----------------------- 6 files changed, 38 insertions(+), 64 deletions(-) diff --git a/src/ebpf.rs b/src/ebpf.rs index 877408db..9507e8d1 100644 --- a/src/ebpf.rs +++ b/src/ebpf.rs @@ -36,10 +36,6 @@ pub const STACK_PTR_REG: usize = 11; pub const FIRST_SCRATCH_REG: usize = 6; /// Number of scratch registers pub const SCRATCH_REGS: usize = 4; -/// ELF dump instruction offset -/// Instruction numbers typically start at 29 in the ELF dump, use this offset -/// when reporting so that trace aligns with the dump. -pub const ELF_INSN_DUMP_OFFSET: usize = 29; /// Alignment of the memory regions in host address space in bytes pub const HOST_ALIGN: usize = 16; /// Upper half of a pointer is the region index, lower half the virtual address inside that region. diff --git a/src/elf.rs b/src/elf.rs index b9150f60..0442174e 100644 --- a/src/elf.rs +++ b/src/elf.rs @@ -834,9 +834,7 @@ impl Executable { .saturating_add(1) .saturating_add(insn.imm as isize); if target_pc < 0 || target_pc >= instruction_count as isize { - return Err(ElfError::RelativeJumpOutOfBounds( - i.saturating_add(ebpf::ELF_INSN_DUMP_OFFSET), - )); + return Err(ElfError::RelativeJumpOutOfBounds(i)); } let name = if config.enable_symbol_and_section_labels { format!("function_{target_pc}") @@ -1101,12 +1099,7 @@ impl Executable { { return Err(ElfError::UnresolvedSymbol( String::from_utf8_lossy(name).to_string(), - r_offset - .checked_div(ebpf::INSN_SIZE) - .and_then(|offset| { - offset.checked_add(ebpf::ELF_INSN_DUMP_OFFSET) - }) - .unwrap_or(ebpf::ELF_INSN_DUMP_OFFSET), + r_offset.checked_div(ebpf::INSN_SIZE).unwrap_or(0), r_offset, )); } @@ -1951,7 +1944,7 @@ mod test { } #[test] - #[should_panic(expected = "validation failed: RelativeJumpOutOfBounds(38)")] + #[should_panic(expected = "validation failed: RelativeJumpOutOfBounds(9)")] fn test_relative_call_oob_backward() { let mut elf_bytes = std::fs::read("tests/elfs/relative_call.so").expect("failed to read elf file"); @@ -1960,7 +1953,7 @@ mod test { } #[test] - #[should_panic(expected = "validation failed: RelativeJumpOutOfBounds(41)")] + #[should_panic(expected = "validation failed: RelativeJumpOutOfBounds(12)")] fn test_relative_call_oob_forward() { let mut elf_bytes = std::fs::read("tests/elfs/relative_call.so").expect("failed to read elf file"); diff --git a/src/static_analysis.rs b/src/static_analysis.rs index 67fe9c99..88ef1f2d 100644 --- a/src/static_analysis.rs +++ b/src/static_analysis.rs @@ -485,7 +485,7 @@ impl<'a> Analysis<'a> { "{:5?} {:016X?} {:5?}: {}", index, &entry[0..11], - pc + ebpf::ELF_INSN_DUMP_OFFSET, + pc, self.disassemble_instruction(insn), )?; } diff --git a/src/verifier.rs b/src/verifier.rs index a3a14a81..63cf1129 100644 --- a/src/verifier.rs +++ b/src/verifier.rs @@ -104,10 +104,6 @@ pub trait Verifier { ) -> Result<(), VerifierError>; } -fn adj_insn_ptr(insn_ptr: usize) -> usize { - insn_ptr + ebpf::ELF_INSN_DUMP_OFFSET -} - fn check_prog_len(prog: &[u8]) -> Result<(), VerifierError> { if prog.len() % ebpf::INSN_SIZE != 0 { return Err(VerifierError::ProgramLengthNotMultiple); @@ -120,7 +116,7 @@ fn check_prog_len(prog: &[u8]) -> Result<(), VerifierError> { fn check_imm_nonzero(insn: &ebpf::Insn, insn_ptr: usize) -> Result<(), VerifierError> { if insn.imm == 0 { - return Err(VerifierError::DivisionByZero(adj_insn_ptr(insn_ptr))); + return Err(VerifierError::DivisionByZero(insn_ptr)); } Ok(()) } @@ -128,9 +124,7 @@ fn check_imm_nonzero(insn: &ebpf::Insn, insn_ptr: usize) -> Result<(), VerifierE fn check_imm_endian(insn: &ebpf::Insn, insn_ptr: usize) -> Result<(), VerifierError> { match insn.imm { 16 | 32 | 64 => Ok(()), - _ => Err(VerifierError::UnsupportedLEBEArgument(adj_insn_ptr( - insn_ptr, - ))), + _ => Err(VerifierError::UnsupportedLEBEArgument(insn_ptr)), } } @@ -141,7 +135,7 @@ fn check_load_dw(prog: &[u8], insn_ptr: usize) -> Result<(), VerifierError> { } let next_insn = ebpf::get_insn(prog, insn_ptr + 1); if next_insn.opc != 0 { - return Err(VerifierError::IncompleteLDDW(adj_insn_ptr(insn_ptr))); + return Err(VerifierError::IncompleteLDDW(insn_ptr)); } Ok(()) } @@ -157,14 +151,14 @@ fn check_jmp_offset( if dst_insn_ptr < 0 || !function_range.contains(&(dst_insn_ptr as usize)) { return Err(VerifierError::JumpOutOfCode( dst_insn_ptr as usize, - adj_insn_ptr(insn_ptr), + insn_ptr, )); } let dst_insn = ebpf::get_insn(prog, dst_insn_ptr as usize); if dst_insn.opc == 0 { return Err(VerifierError::JumpToMiddleOfLDDW( dst_insn_ptr as usize, - adj_insn_ptr(insn_ptr), + insn_ptr, )); } Ok(()) @@ -177,16 +171,14 @@ fn check_registers( sbpf_version: &SBPFVersion, ) -> Result<(), VerifierError> { if insn.src > 10 { - return Err(VerifierError::InvalidSourceRegister(adj_insn_ptr(insn_ptr))); + return Err(VerifierError::InvalidSourceRegister(insn_ptr)); } match (insn.dst, store) { (0..=9, _) | (10, true) => Ok(()), (11, _) if sbpf_version.dynamic_stack_frames() && insn.opc == ebpf::ADD64_IMM => Ok(()), - (10, false) => Err(VerifierError::CannotWriteR10(adj_insn_ptr(insn_ptr))), - (_, _) => Err(VerifierError::InvalidDestinationRegister(adj_insn_ptr( - insn_ptr, - ))), + (10, false) => Err(VerifierError::CannotWriteR10(insn_ptr)), + (_, _) => Err(VerifierError::InvalidDestinationRegister(insn_ptr)), } } @@ -195,9 +187,7 @@ fn check_imm_shift(insn: &ebpf::Insn, insn_ptr: usize, imm_bits: u64) -> Result< let shift_by = insn.imm as u64; if insn.imm < 0 || shift_by >= imm_bits { return Err(VerifierError::ShiftWithOverflow( - shift_by, - imm_bits, - adj_insn_ptr(insn_ptr), + shift_by, imm_bits, insn_ptr, )); } Ok(()) @@ -216,7 +206,7 @@ fn check_callx_register( insn.imm }; if !(0..=10).contains(®) || (reg == 10 && config.reject_callx_r10) { - return Err(VerifierError::InvalidRegister(adj_insn_ptr(insn_ptr))); + return Err(VerifierError::InvalidRegister(insn_ptr)); } Ok(()) } @@ -387,7 +377,7 @@ impl Verifier for RequisiteVerifier { ebpf::EXIT => {}, _ => { - return Err(VerifierError::UnknownOpCode(insn.opc, adj_insn_ptr(insn_ptr))); + return Err(VerifierError::UnknownOpCode(insn.opc, insn_ptr)); } } @@ -398,7 +388,7 @@ impl Verifier for RequisiteVerifier { // insn_ptr should now be equal to number of instructions. if insn_ptr != prog.len() / ebpf::INSN_SIZE { - return Err(VerifierError::JumpOutOfCode(adj_insn_ptr(insn_ptr), adj_insn_ptr(insn_ptr))); + return Err(VerifierError::JumpOutOfCode(insn_ptr, insn_ptr)); } Ok(()) diff --git a/tests/execution.rs b/tests/execution.rs index ddc30ab8..f7ef5361 100644 --- a/tests/execution.rs +++ b/tests/execution.rs @@ -2881,7 +2881,7 @@ fn test_err_unresolved_syscall_reloc_64_32() { file.read_to_end(&mut elf).unwrap(); assert_error!( Executable::::from_elf(&elf, Arc::new(loader)), - "UnresolvedSymbol(\"log\", 68, 312)" + "UnresolvedSymbol(\"log\", 39, 312)" ); } diff --git a/tests/verifier.rs b/tests/verifier.rs index ff56b6d3..9146f798 100644 --- a/tests/verifier.rs +++ b/tests/verifier.rs @@ -100,7 +100,7 @@ fn test_verifier_fail() { } #[test] -#[should_panic(expected = "DivisionByZero(30)")] +#[should_panic(expected = "DivisionByZero(1)")] fn test_verifier_err_div_by_zero_imm() { let executable = assemble::( " @@ -114,7 +114,7 @@ fn test_verifier_err_div_by_zero_imm() { } #[test] -#[should_panic(expected = "UnsupportedLEBEArgument(29)")] +#[should_panic(expected = "UnsupportedLEBEArgument(0)")] fn test_verifier_err_endian_size() { let prog = &[ 0xdc, 0x01, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, // @@ -132,7 +132,7 @@ fn test_verifier_err_endian_size() { } #[test] -#[should_panic(expected = "IncompleteLDDW(29)")] +#[should_panic(expected = "IncompleteLDDW(0)")] fn test_verifier_err_incomplete_lddw() { // Note: ubpf has test-err-incomplete-lddw2, which is the same let prog = &[ @@ -168,7 +168,7 @@ fn test_verifier_err_invalid_reg_dst() { ) .unwrap(); let result = executable.verify::(); - assert_error!(result, "VerifierError(InvalidDestinationRegister(29))"); + assert_error!(result, "VerifierError(InvalidDestinationRegister(0))"); } } @@ -191,7 +191,7 @@ fn test_verifier_err_invalid_reg_src() { ) .unwrap(); let result = executable.verify::(); - assert_error!(result, "VerifierError(InvalidSourceRegister(29))"); + assert_error!(result, "VerifierError(InvalidSourceRegister(0))"); } } @@ -215,7 +215,7 @@ fn test_verifier_resize_stack_ptr_success() { } #[test] -#[should_panic(expected = "JumpToMiddleOfLDDW(2, 29)")] +#[should_panic(expected = "JumpToMiddleOfLDDW(2, 0)")] fn test_verifier_err_jmp_lddw() { let executable = assemble::( " @@ -257,7 +257,7 @@ fn test_verifier_err_function_fallthrough() { } #[test] -#[should_panic(expected = "JumpOutOfCode(3, 29)")] +#[should_panic(expected = "JumpOutOfCode(3, 0)")] fn test_verifier_err_jmp_out() { let executable = assemble::( " @@ -270,7 +270,7 @@ fn test_verifier_err_jmp_out() { } #[test] -#[should_panic(expected = "JumpOutOfCode(18446744073709551615, 29)")] +#[should_panic(expected = "JumpOutOfCode(18446744073709551615, 0)")] fn test_verifier_err_jmp_out_start() { let executable = assemble::( " @@ -283,7 +283,7 @@ fn test_verifier_err_jmp_out_start() { } #[test] -#[should_panic(expected = "UnknownOpCode(6, 29)")] +#[should_panic(expected = "UnknownOpCode(6, 0)")] fn test_verifier_err_unknown_opcode() { let prog = &[ 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // @@ -300,7 +300,7 @@ fn test_verifier_err_unknown_opcode() { } #[test] -#[should_panic(expected = "CannotWriteR10(29)")] +#[should_panic(expected = "CannotWriteR10(0)")] fn test_verifier_err_write_r10() { let executable = assemble::( " @@ -317,25 +317,25 @@ fn test_verifier_err_all_shift_overflows() { let testcases = [ // lsh32_imm ("lsh32 r0, 16", Ok(())), - ("lsh32 r0, 32", Err("ShiftWithOverflow(32, 32, 29)")), - ("lsh32 r0, 64", Err("ShiftWithOverflow(64, 32, 29)")), + ("lsh32 r0, 32", Err("ShiftWithOverflow(32, 32, 0)")), + ("lsh32 r0, 64", Err("ShiftWithOverflow(64, 32, 0)")), // rsh32_imm ("rsh32 r0, 16", Ok(())), - ("rsh32 r0, 32", Err("ShiftWithOverflow(32, 32, 29)")), - ("rsh32 r0, 64", Err("ShiftWithOverflow(64, 32, 29)")), + ("rsh32 r0, 32", Err("ShiftWithOverflow(32, 32, 0)")), + ("rsh32 r0, 64", Err("ShiftWithOverflow(64, 32, 0)")), // arsh32_imm ("arsh32 r0, 16", Ok(())), - ("arsh32 r0, 32", Err("ShiftWithOverflow(32, 32, 29)")), - ("arsh32 r0, 64", Err("ShiftWithOverflow(64, 32, 29)")), + ("arsh32 r0, 32", Err("ShiftWithOverflow(32, 32, 0)")), + ("arsh32 r0, 64", Err("ShiftWithOverflow(64, 32, 0)")), // lsh64_imm ("lsh64 r0, 32", Ok(())), - ("lsh64 r0, 64", Err("ShiftWithOverflow(64, 64, 29)")), + ("lsh64 r0, 64", Err("ShiftWithOverflow(64, 64, 0)")), // rsh64_imm ("rsh64 r0, 32", Ok(())), - ("rsh64 r0, 64", Err("ShiftWithOverflow(64, 64, 29)")), + ("rsh64 r0, 64", Err("ShiftWithOverflow(64, 64, 0)")), // arsh64_imm ("arsh64 r0, 32", Ok(())), - ("arsh64 r0, 64", Err("ShiftWithOverflow(64, 64, 29)")), + ("arsh64 r0, 64", Err("ShiftWithOverflow(64, 64, 0)")), ]; for (overflowing_instruction, expected) in testcases { @@ -377,12 +377,7 @@ fn test_sdiv_disabled() { if enable_sbpf_v2 { assert!(result.is_ok()); } else { - assert_error!( - result, - "VerifierError(UnknownOpCode({}, {}))", - opc, - ebpf::ELF_INSN_DUMP_OFFSET - ); + assert_error!(result, "VerifierError(UnknownOpCode({}, {}))", opc, 0); } } }