From 1709f6e6731e04ea506acedbbab5d6308605a23f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Wed, 6 Dec 2023 10:59:46 +0100 Subject: [PATCH 1/2] General cleanup --- src/aligned_memory.rs | 3 +-- src/assembler.rs | 25 +++++++++++++------------ src/elf_parser/mod.rs | 2 +- src/error.rs | 13 +------------ src/memory_region.rs | 14 ++------------ src/program.rs | 2 +- src/static_analysis.rs | 3 +-- src/syscalls.rs | 12 +++--------- 8 files changed, 23 insertions(+), 51 deletions(-) diff --git a/src/aligned_memory.rs b/src/aligned_memory.rs index a9dcfe56..3247c38c 100644 --- a/src/aligned_memory.rs +++ b/src/aligned_memory.rs @@ -27,7 +27,6 @@ pub struct AlignedMemory { impl AlignedMemory { fn get_mem(max_len: usize) -> (Vec, usize) { let mut mem: Vec = Vec::with_capacity(max_len.saturating_add(ALIGN)); - mem.push(0); let align_offset = mem.as_ptr().align_offset(ALIGN); mem.resize(align_offset, 0); (mem, align_offset) @@ -122,7 +121,7 @@ impl AlignedMemory { _ => { return Err(std::io::Error::new( std::io::ErrorKind::InvalidInput, - "aligned memory resize failed", + "aligned memory fill_write failed", )) } }; diff --git a/src/assembler.rs b/src/assembler.rs index 97e6721e..ff124b70 100644 --- a/src/assembler.rs +++ b/src/assembler.rs @@ -254,6 +254,17 @@ fn insn(opc: u8, dst: i64, src: i64, off: i64, imm: i64) -> Result }) } +fn resolve_label( + insn_ptr: usize, + labels: &HashMap<&str, usize>, + label: &str, +) -> Result { + labels + .get(label) + .map(|target_pc| *target_pc as i64 - insn_ptr as i64 - 1) + .ok_or_else(|| format!("Label not found {label}")) +} + /// Parse assembly source and translate to binary. /// /// # Examples @@ -299,16 +310,6 @@ pub fn assemble( } else { SBPFVersion::V1 }; - fn resolve_label( - insn_ptr: usize, - labels: &HashMap<&str, usize>, - label: &str, - ) -> Result { - labels - .get(label) - .map(|target_pc| *target_pc as i64 - insn_ptr as i64 - 1) - .ok_or_else(|| format!("Label not found {label}")) - } let statements = parse(src)?; let instruction_map = make_instruction_map(); @@ -321,7 +322,7 @@ pub fn assemble( Statement::Label { name } => { if name.starts_with("function_") || name == "entrypoint" { function_registry - .register_function(insn_ptr as u32, name.as_bytes().to_vec(), insn_ptr) + .register_function(insn_ptr as u32, name.as_bytes(), insn_ptr) .map_err(|_| format!("Label hash collision {name}"))?; } labels.insert(name.as_str(), insn_ptr); @@ -372,7 +373,7 @@ pub fn assemble( function_registry .register_function( target_pc as u32, - label.as_bytes().to_vec(), + label.as_bytes(), target_pc as usize, ) .map_err(|_| format!("Label hash collision {name}"))?; diff --git a/src/elf_parser/mod.rs b/src/elf_parser/mod.rs index bddcfeb4..288c35cc 100644 --- a/src/elf_parser/mod.rs +++ b/src/elf_parser/mod.rs @@ -135,7 +135,7 @@ impl<'a> Elf64<'a> { let section_header_table = slice_from_bytes::(elf_bytes, section_header_table_range.clone())?; section_header_table - .get(0) + .first() .filter(|section_header| section_header.sh_type == SHT_NULL) .ok_or(ElfParserError::InvalidSectionHeader)?; diff --git a/src/error.rs b/src/error.rs index 2e1fc445..989acba0 100644 --- a/src/error.rs +++ b/src/error.rs @@ -4,18 +4,7 @@ // the MIT license , at your option. This file may not be // copied, modified, or distributed except according to those terms. -//! This module contains all the definitions related to eBPF, and some functions permitting to -//! manipulate eBPF instructions. -//! -//! The number of bytes in an instruction, the maximum number of instructions in a program, and -//! also all operation codes are defined here as constants. -//! -//! The structure for an instruction used by this crate, as well as the function to extract it from -//! a program, is also defined in the module. -//! -//! To learn more about these instructions, see the Linux kernel documentation: -//! , or for a shorter version of -//! the list of the operation codes: +//! This module contains error and result types use { crate::{elf::ElfError, memory_region::AccessType, verifier::VerifierError}, diff --git a/src/memory_region.rs b/src/memory_region.rs index 260596cc..1e1611b7 100644 --- a/src/memory_region.rs +++ b/src/memory_region.rs @@ -104,12 +104,7 @@ impl MemoryRegion { /// Creates a new writable MemoryRegion from a mutable slice pub fn new_writable(slice: &mut [u8], vm_addr: u64) -> Self { - Self::new( - unsafe { std::mem::transmute::<&mut [u8], &[u8]>(slice) }, - vm_addr, - 0, - MemoryState::Writable, - ) + Self::new(&*slice, vm_addr, 0, MemoryState::Writable) } /// Creates a new copy on write MemoryRegion. @@ -121,12 +116,7 @@ impl MemoryRegion { /// Creates a new writable gapped MemoryRegion from a mutable slice pub fn new_writable_gapped(slice: &mut [u8], vm_addr: u64, vm_gap_size: u64) -> Self { - Self::new( - unsafe { std::mem::transmute::<&mut [u8], &[u8]>(slice) }, - vm_addr, - vm_gap_size, - MemoryState::Writable, - ) + Self::new(&*slice, vm_addr, vm_gap_size, MemoryState::Writable) } /// Convert a virtual machine address into a host address diff --git a/src/program.rs b/src/program.rs index 3e6aaef2..c1c138ea 100644 --- a/src/program.rs +++ b/src/program.rs @@ -167,7 +167,7 @@ impl FunctionRegistry { /// Iterate over all keys pub fn keys(&self) -> impl Iterator + '_ { - self.map.keys().cloned() + self.map.keys().copied() } /// Iterate over all entries diff --git a/src/static_analysis.rs b/src/static_analysis.rs index 88ef1f2d..737f34ff 100644 --- a/src/static_analysis.rs +++ b/src/static_analysis.rs @@ -553,8 +553,7 @@ impl<'a> Analysis<'a> { format!("{}", html_escape(&desc)) } }) - .collect::>() - .join("") + .collect::() )?; if let Some(dynamic_analysis) = dynamic_analysis { if let Some(recorded_edges) = dynamic_analysis.edges.get(&cfg_node_start) { diff --git a/src/syscalls.rs b/src/syscalls.rs index b21930dc..c597d9b2 100644 --- a/src/syscalls.rs +++ b/src/syscalls.rs @@ -158,16 +158,10 @@ declare_builtin_function!( let host_addr: Result = memory_mapping.map(AccessType::Load, vm_addr, len).into(); let host_addr = host_addr?; - let c_buf: *const i8 = host_addr as *const i8; unsafe { - for i in 0..len { - let c = std::ptr::read(c_buf.offset(i as isize)); - if c == 0 { - break; - } - } - let message = from_utf8(from_raw_parts(host_addr as *const u8, len as usize)) - .unwrap_or("Invalid UTF-8 String"); + let c_buf = from_raw_parts(host_addr as *const u8, len as usize); + let len = c_buf.iter().position(|c| *c == 0).unwrap_or(len as usize); + let message = from_utf8(&c_buf[0..len]).unwrap_or("Invalid UTF-8 String"); println!("log: {message}"); } Ok(0) From 13dad2492a43cae9a0d52d8189b04e108caca0f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexander=20Mei=C3=9Fner?= Date: Wed, 6 Dec 2023 11:18:06 +0100 Subject: [PATCH 2/2] Adds deny(clippy::ptr_as_ptr). --- src/elf.rs | 4 ++-- src/error.rs | 2 +- src/jit.rs | 13 +++++++------ src/lib.rs | 1 + src/memory_management.rs | 10 +++++----- src/memory_region.rs | 4 ++-- src/program.rs | 2 +- src/vm.rs | 6 ++++-- 8 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/elf.rs b/src/elf.rs index 0442174e..14b442c3 100644 --- a/src/elf.rs +++ b/src/elf.rs @@ -1219,7 +1219,7 @@ mod test { let write_header = |header: Elf64Ehdr| unsafe { let mut bytes = elf_bytes.clone(); - std::ptr::write(bytes.as_mut_ptr() as *mut Elf64Ehdr, header); + std::ptr::write(bytes.as_mut_ptr().cast::(), header); bytes }; @@ -1322,7 +1322,7 @@ mod test { let write_header = |header: Elf64Ehdr| unsafe { let mut bytes = elf_bytes.clone(); - std::ptr::write(bytes.as_mut_ptr() as *mut Elf64Ehdr, header); + std::ptr::write(bytes.as_mut_ptr().cast::(), header); bytes }; diff --git a/src/error.rs b/src/error.rs index 989acba0..b10ac5bb 100644 --- a/src/error.rs +++ b/src/error.rs @@ -145,7 +145,7 @@ impl StableResult { allow(dead_code) )] pub(crate) fn discriminant(&self) -> u64 { - unsafe { *(self as *const _ as *const u64) } + unsafe { *std::ptr::addr_of!(*self).cast::() } } } diff --git a/src/jit.rs b/src/jit.rs index c68bb8b6..75399bde 100644 --- a/src/jit.rs +++ b/src/jit.rs @@ -47,7 +47,7 @@ impl JitProgram { let raw = allocate_pages(pc_loc_table_size + over_allocated_code_size)?; Ok(Self { page_size, - pc_section: std::slice::from_raw_parts_mut(raw as *mut usize, pc), + pc_section: std::slice::from_raw_parts_mut(raw.cast::(), pc), text_section: std::slice::from_raw_parts_mut( raw.add(pc_loc_table_size), over_allocated_code_size, @@ -80,7 +80,7 @@ impl JitProgram { self.text_section = std::slice::from_raw_parts_mut(raw.add(pc_loc_table_size), text_section_usage); protect_pages( - self.pc_section.as_mut_ptr() as *mut u8, + self.pc_section.as_mut_ptr().cast::(), pc_loc_table_size, false, )?; @@ -119,7 +119,7 @@ impl JitProgram { "pop rbp", "pop rbx", host_stack_pointer = in(reg) &mut vm.host_stack_pointer, - inlateout("rdi") (vm as *mut _ as *mut u64).offset(get_runtime_environment_key() as isize) => _, + inlateout("rdi") std::ptr::addr_of_mut!(*vm).cast::().offset(get_runtime_environment_key() as isize) => _, inlateout("rax") (vm.previous_instruction_meter as i64).wrapping_add(registers[11] as i64) => _, inlateout("r10") self.pc_section[registers[11] as usize] => _, inlateout("r11") ®isters => _, @@ -1269,7 +1269,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { } fn emit_set_exception_kind(&mut self, err: EbpfError) { - let err_kind = unsafe { *(&err as *const _ as *const u64) }; + let err_kind = unsafe { *std::ptr::addr_of!(err).cast::() }; let err_discriminant = ProgramResult::Err(err).discriminant(); self.emit_ins(X86Instruction::lea(OperandSize::S64, REGISTER_PTR_TO_VM, REGISTER_OTHER_SCRATCH, Some(X86IndirectAccess::Offset(self.slot_in_vm(RuntimeEnvironmentSlot::ProgramResult))))); self.emit_ins(X86Instruction::store_immediate(OperandSize::S64, REGISTER_OTHER_SCRATCH, X86IndirectAccess::Offset(0), err_discriminant as i64)); // result.discriminant = err_discriminant; @@ -1615,8 +1615,9 @@ mod tests { ($env:expr, $entry:ident, $slot:ident) => { assert_eq!( unsafe { - (&$env.$entry as *const _ as *const u64) - .offset_from(&$env as *const _ as *const u64) as usize + std::ptr::addr_of!($env.$entry) + .cast::() + .offset_from(std::ptr::addr_of!($env).cast::()) as usize }, RuntimeEnvironmentSlot::$slot as usize, ); diff --git a/src/lib.rs b/src/lib.rs index ae4c5652..def5dcf5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,6 +16,7 @@ html_favicon_url = "https://raw.githubusercontent.com/qmonnet/rbpf/master/misc/rbpf.ico" )] #![deny(clippy::arithmetic_side_effects)] +#![deny(clippy::ptr_as_ptr)] extern crate byteorder; extern crate combine; diff --git a/src/memory_management.rs b/src/memory_management.rs index 9c7bac5e..7606db3c 100644 --- a/src/memory_management.rs +++ b/src/memory_management.rs @@ -113,16 +113,16 @@ pub unsafe fn allocate_pages(size_in_bytes: usize) -> Result<*mut u8, EbpfError> winnt::MEM_RESERVE | winnt::MEM_COMMIT, winnt::PAGE_READWRITE, ); - Ok(raw as *mut u8) + Ok(raw.cast::()) } pub unsafe fn free_pages(raw: *mut u8, size_in_bytes: usize) -> Result<(), EbpfError> { #[cfg(not(target_os = "windows"))] - libc_error_guard!(munmap, raw as *mut _, size_in_bytes); + libc_error_guard!(munmap, raw.cast::(), size_in_bytes); #[cfg(target_os = "windows")] winapi_error_guard!( VirtualFree, - raw as *mut _, + raw.cast::(), size_in_bytes, winnt::MEM_RELEASE, // winnt::MEM_DECOMMIT ); @@ -138,7 +138,7 @@ pub unsafe fn protect_pages( { libc_error_guard!( mprotect, - raw as *mut _, + raw.cast::(), size_in_bytes, if executable_flag { libc::PROT_EXEC | libc::PROT_READ @@ -153,7 +153,7 @@ pub unsafe fn protect_pages( let ptr_old: *mut minwindef::DWORD = &mut old; winapi_error_guard!( VirtualProtect, - raw as *mut _, + raw.cast::(), size_in_bytes, if executable_flag { winnt::PAGE_EXECUTE_READ diff --git a/src/memory_region.rs b/src/memory_region.rs index 1e1611b7..e8648557 100644 --- a/src/memory_region.rs +++ b/src/memory_region.rs @@ -389,7 +389,7 @@ impl<'a> UnalignedMemoryMapping<'a> { let initial_len = len; let initial_vm_addr = vm_addr; let mut value = 0u64; - let mut ptr = &mut value as *mut _ as *mut u8; + let mut ptr = std::ptr::addr_of_mut!(value).cast::(); while len > 0 { let load_len = len.min(region.vm_addr_end.saturating_sub(vm_addr)); @@ -440,7 +440,7 @@ impl<'a> UnalignedMemoryMapping<'a> { // guaranteed to be unique. let cache = unsafe { &mut *self.cache.get() }; - let mut src = &value as *const _ as *const u8; + let mut src = std::ptr::addr_of!(value).cast::(); let mut region = match self.find_region(cache, vm_addr) { Some(region) if ensure_writable_region(region, &self.cow_cb) => { diff --git a/src/program.rs b/src/program.rs index c1c138ea..6605814d 100644 --- a/src/program.rs +++ b/src/program.rs @@ -324,7 +324,7 @@ macro_rules! declare_builtin_function { ) { use $crate::vm::ContextObject; let vm = unsafe { - &mut *(($vm as *mut u64).offset(-($crate::vm::get_runtime_environment_key() as isize)) as *mut $crate::vm::EbpfVm<$ContextObject>) + &mut *($vm.cast::().offset(-($crate::vm::get_runtime_environment_key() as isize)).cast::<$crate::vm::EbpfVm<$ContextObject>>()) }; let config = vm.loader.get_config(); if config.enable_instruction_meter { diff --git a/src/vm.rs b/src/vm.rs index 9f6dbd0b..40c222a7 100644 --- a/src/vm.rs +++ b/src/vm.rs @@ -426,8 +426,10 @@ impl<'a, C: ContextObject> EbpfVm<'a, C> { pub fn invoke_function(&mut self, function: BuiltinFunction) { function( unsafe { - (self as *mut _ as *mut u64).offset(get_runtime_environment_key() as isize) - as *mut _ + std::ptr::addr_of_mut!(*self) + .cast::() + .offset(get_runtime_environment_key() as isize) + .cast::() }, self.registers[1], self.registers[2],