From 75249fd3189d624c3d09bbc9a730dc81ce4472c2 Mon Sep 17 00:00:00 2001 From: Lucas Steuernagel Date: Fri, 18 Oct 2024 15:11:42 -0300 Subject: [PATCH 1/8] Update BuiltinProgram --- src/disassembler.rs | 2 +- src/elf.rs | 7 ++-- src/interpreter.rs | 2 +- src/jit.rs | 2 +- src/program.rs | 75 +++++++++++++++++++++++++++++++++--------- src/static_analysis.rs | 2 +- 6 files changed, 68 insertions(+), 22 deletions(-) diff --git a/src/disassembler.rs b/src/disassembler.rs index 97edc391..23dc1b87 100644 --- a/src/disassembler.rs +++ b/src/disassembler.rs @@ -271,7 +271,7 @@ pub fn disassemble_instruction( function_name } else { name = "syscall"; - loader.get_function_registry().lookup_by_key(insn.imm as u32).map(|(function_name, _)| String::from_utf8_lossy(function_name).to_string()).unwrap_or_else(|| "[invalid]".to_string()) + loader.get_sparse_function_registry().lookup_by_key(insn.imm as u32).map(|(function_name, _)| String::from_utf8_lossy(function_name).to_string()).unwrap_or_else(|| "[invalid]".to_string()) }; desc = format!("{name} {function_name}"); }, diff --git a/src/elf.rs b/src/elf.rs index aecc47f4..584b2f02 100644 --- a/src/elf.rs +++ b/src/elf.rs @@ -315,7 +315,7 @@ impl Executable { self.get_config(), self.get_sbpf_version(), self.get_function_registry(), - self.loader.get_function_registry(), + self.loader.get_sparse_function_registry(), )?; Ok(()) } @@ -1071,7 +1071,10 @@ impl Executable { .entry(symbol.st_name) .or_insert_with(|| ebpf::hash_symbol_name(name)); if config.reject_broken_elfs - && loader.get_function_registry().lookup_by_key(hash).is_none() + && loader + .get_sparse_function_registry() + .lookup_by_key(hash) + .is_none() { return Err(ElfError::UnresolvedSymbol( String::from_utf8_lossy(name).to_string(), diff --git a/src/interpreter.rs b/src/interpreter.rs index 34ea6dd8..26e8c800 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -536,7 +536,7 @@ impl<'a, 'b, C: ContextObject> Interpreter<'a, 'b, C> { }; if external { - if let Some((_function_name, function)) = self.executable.get_loader().get_function_registry().lookup_by_key(insn.imm as u32) { + if let Some((_function_name, function)) = self.executable.get_loader().get_sparse_function_registry().lookup_by_key(insn.imm as u32) { resolved = true; self.vm.due_insn_count = self.vm.previous_instruction_meter - self.vm.due_insn_count; diff --git a/src/jit.rs b/src/jit.rs index 1f48034e..c961828f 100644 --- a/src/jit.rs +++ b/src/jit.rs @@ -717,7 +717,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { }; if external { - if let Some((_function_name, function)) = self.executable.get_loader().get_function_registry().lookup_by_key(insn.imm as u32) { + if let Some((_function_name, function)) = self.executable.get_loader().get_sparse_function_registry().lookup_by_key(insn.imm as u32) { self.emit_validate_and_profile_instruction_count(false, Some(0)); self.emit_ins(X86Instruction::load_immediate(OperandSize::S64, REGISTER_SCRATCH, function as usize as i64)); self.emit_ins(X86Instruction::call_immediate(self.relative_to_anchor(ANCHOR_EXTERNAL_FUNCTION_CALL, 5))); diff --git a/src/program.rs b/src/program.rs index 470a3830..e31293bb 100644 --- a/src/program.rs +++ b/src/program.rs @@ -153,7 +153,11 @@ impl FunctionRegistry { } else { ebpf::hash_symbol_name(&usize::from(value).to_le_bytes()) }; - if loader.get_function_registry().lookup_by_key(hash).is_some() { + if loader + .get_sparse_function_registry() + .lookup_by_key(hash) + .is_some() + { return Err(ElfError::SymbolHashCollision(hash)); } hash @@ -228,13 +232,17 @@ pub type BuiltinFunction = fn(*mut EbpfVm, u64, u64, u64, u64, u64); pub struct BuiltinProgram { /// Holds the Config if this is a loader program config: Option>, - /// Function pointers by symbol - functions: FunctionRegistry>, + /// Function pointers by symbol with sparse indexing + sparse_registry: FunctionRegistry>, + /// Function pointers by symbol with dense indexing + dense_registry: FunctionRegistry>, } impl PartialEq for BuiltinProgram { fn eq(&self, other: &Self) -> bool { - self.config.eq(&other.config) && self.functions.eq(&other.functions) + self.config.eq(&other.config) + && self.sparse_registry.eq(&other.sparse_registry) + && self.dense_registry.eq(&other.dense_registry) } } @@ -243,7 +251,8 @@ impl BuiltinProgram { pub fn new_loader(config: Config, functions: FunctionRegistry>) -> Self { Self { config: Some(Box::new(config)), - functions, + sparse_registry: functions, + dense_registry: FunctionRegistry::default(), } } @@ -251,7 +260,8 @@ impl BuiltinProgram { pub fn new_builtin(functions: FunctionRegistry>) -> Self { Self { config: None, - functions, + sparse_registry: functions, + dense_registry: FunctionRegistry::default(), } } @@ -259,7 +269,18 @@ impl BuiltinProgram { pub fn new_mock() -> Self { Self { config: Some(Box::default()), - functions: FunctionRegistry::default(), + sparse_registry: FunctionRegistry::default(), + dense_registry: FunctionRegistry::default(), + } + } + + /// Create a new loader with both dense and sparse function registrations + /// Use `BuiltinProgram::register_function` to register. + pub fn new_loader_with_dense_registration(config: Config) -> Self { + Self { + config: Some(Box::new(config)), + sparse_registry: FunctionRegistry::default(), + dense_registry: FunctionRegistry::default(), } } @@ -269,8 +290,8 @@ impl BuiltinProgram { } /// Get the function registry - pub fn get_function_registry(&self) -> &FunctionRegistry> { - &self.functions + pub fn get_sparse_function_registry(&self) -> &FunctionRegistry> { + &self.sparse_registry } /// Calculate memory size @@ -281,18 +302,40 @@ impl BuiltinProgram { } else { 0 }) - .saturating_add(self.functions.mem_size()) + .saturating_add(self.sparse_registry.mem_size()) + .saturating_add(self.dense_registry.mem_size()) + } + + /// Register a function both in the sparse and dense registries + pub fn register_function( + &mut self, + name: &str, + value: BuiltinFunction, + dense_key: u32, + ) -> Result<(), ElfError> { + self.sparse_registry.register_function_hashed(name, value)?; + self.dense_registry + .register_function(dense_key, name, value) } } impl std::fmt::Debug for BuiltinProgram { fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> { - writeln!(f, "{:?}", unsafe { - // `derive(Debug)` does not know that `C: ContextObject` does not need to implement `Debug` - std::mem::transmute::<&FunctionRegistry>, &FunctionRegistry>( - &self.functions, - ) - })?; + unsafe { + writeln!( + f, + "sparse: {:?}\n dense: {:?}", + // `derive(Debug)` does not know that `C: ContextObject` does not need to implement `Debug` + std::mem::transmute::< + &FunctionRegistry>, + &FunctionRegistry, + >(&self.sparse_registry,), + std::mem::transmute::< + &FunctionRegistry>, + &FunctionRegistry, + >(&self.dense_registry,) + )?; + } Ok(()) } } diff --git a/src/static_analysis.rs b/src/static_analysis.rs index aa5d0ae3..ab73c6cc 100644 --- a/src/static_analysis.rs +++ b/src/static_analysis.rs @@ -236,7 +236,7 @@ impl<'a> Analysis<'a> { if let Some((function_name, _function)) = self .executable .get_loader() - .get_function_registry() + .get_sparse_function_registry() .lookup_by_key(insn.imm as u32) { if function_name == b"abort" { From 8390b3e17ca59e21922206290cf37f8026ee90a5 Mon Sep 17 00:00:00 2001 From: Lucas Steuernagel Date: Fri, 18 Oct 2024 18:34:52 -0300 Subject: [PATCH 2/8] Add syscall instruction to verifier --- src/elf.rs | 2 +- src/program.rs | 7 ++++- src/verifier.rs | 22 ++++++++++++--- tests/execution.rs | 67 +++++++++++++++++++++++++++++++++++++++++++++- tests/verifier.rs | 16 +++++------ 5 files changed, 98 insertions(+), 16 deletions(-) diff --git a/src/elf.rs b/src/elf.rs index 584b2f02..6250d778 100644 --- a/src/elf.rs +++ b/src/elf.rs @@ -315,7 +315,7 @@ impl Executable { self.get_config(), self.get_sbpf_version(), self.get_function_registry(), - self.loader.get_sparse_function_registry(), + self.loader.get_dense_function_registry(), )?; Ok(()) } diff --git a/src/program.rs b/src/program.rs index e31293bb..48041a3b 100644 --- a/src/program.rs +++ b/src/program.rs @@ -289,11 +289,16 @@ impl BuiltinProgram { self.config.as_ref().unwrap() } - /// Get the function registry + /// Get the sparse function registry pub fn get_sparse_function_registry(&self) -> &FunctionRegistry> { &self.sparse_registry } + /// Get the dense function registry + pub fn get_dense_function_registry(&self) -> &FunctionRegistry> { + &self.dense_registry + } + /// Calculate memory size pub fn mem_size(&self) -> usize { std::mem::size_of::() diff --git a/src/verifier.rs b/src/verifier.rs index 3e0e6151..3cbee58b 100644 --- a/src/verifier.rs +++ b/src/verifier.rs @@ -73,6 +73,9 @@ pub enum VerifierError { /// Invalid function #[error("Invalid function at instruction {0}")] InvalidFunction(usize), + /// Invalid syscall + #[error("Invalid syscall code {0}")] + InvalidSyscall(u32), } /// eBPF Verifier @@ -157,6 +160,7 @@ fn check_jmp_offset( fn check_call_target( key: u32, function_registry: &FunctionRegistry, + error: VerifierError, ) -> Result<(), VerifierError> where T: Copy, @@ -165,7 +169,7 @@ where function_registry .lookup_by_key(key) .map(|_| ()) - .ok_or(VerifierError::InvalidFunction(key as usize)) + .ok_or(error) } fn check_registers( @@ -386,13 +390,23 @@ impl Verifier for RequisiteVerifier { ebpf::JSLT_REG => { check_jmp_offset(prog, insn_ptr, &function_range)?; }, ebpf::JSLE_IMM => { check_jmp_offset(prog, insn_ptr, &function_range)?; }, ebpf::JSLE_REG => { check_jmp_offset(prog, insn_ptr, &function_range)?; }, - ebpf::CALL_IMM if sbpf_version.static_syscalls() && insn.src != 0 => { check_call_target(insn.imm as u32, function_registry)?; }, - ebpf::CALL_IMM if sbpf_version.static_syscalls() && insn.src == 0 => { check_call_target(insn.imm as u32, syscall_registry)?; }, + ebpf::CALL_IMM if sbpf_version.static_syscalls() => { + check_call_target( + insn.imm as u32, + function_registry, + VerifierError::InvalidFunction(insn.imm as usize) + )?; + }, ebpf::CALL_IMM => {}, ebpf::CALL_REG => { check_callx_register(&insn, insn_ptr, sbpf_version)?; }, ebpf::EXIT if !sbpf_version.static_syscalls() => {}, ebpf::RETURN if sbpf_version.static_syscalls() => {}, - ebpf::SYSCALL if sbpf_version.static_syscalls() => {}, + ebpf::SYSCALL if sbpf_version.static_syscalls() => { + check_call_target( + insn.imm as u32, + syscall_registry, + VerifierError::InvalidSyscall(insn.imm as u32))?; + }, _ => { return Err(VerifierError::UnknownOpCode(insn.opc, insn_ptr)); diff --git a/tests/execution.rs b/tests/execution.rs index 1548af71..f9fbe4be 100644 --- a/tests/execution.rs +++ b/tests/execution.rs @@ -1999,6 +1999,10 @@ fn test_stack1() { #[test] fn test_stack2() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " stb [r10-4], 0x01 @@ -2017,6 +2021,7 @@ fn test_stack2() { syscall bpf_gather_bytes xor r0, 0x2a2a2a2a exit", + config, [], ( "bpf_mem_frob" => syscalls::SyscallMemFrob::vm, @@ -2029,6 +2034,10 @@ fn test_stack2() { #[test] fn test_string_stack() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov r1, 0x78636261 @@ -2059,6 +2068,7 @@ fn test_string_stack() { jeq r1, r6, +1 mov r0, 0x0 exit", + config, [], ( "bpf_str_cmp" => syscalls::SyscallStrCmp::vm, @@ -2367,6 +2377,10 @@ fn test_bpf_to_bpf_scratch_registers() { #[test] fn test_syscall_parameter_on_stack() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov64 r1, r10 @@ -2375,6 +2389,7 @@ fn test_syscall_parameter_on_stack() { syscall bpf_syscall_string mov64 r0, 0x0 exit", + config, [], ( "bpf_syscall_string" => syscalls::SyscallString::vm, @@ -2559,12 +2574,17 @@ fn test_call_save() { #[test] fn test_err_syscall_string() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov64 r1, 0x0 syscall bpf_syscall_string mov64 r0, 0x0 exit", + config, [72, 101, 108, 108, 111], ( "bpf_syscall_string" => syscalls::SyscallString::vm, @@ -2576,12 +2596,17 @@ fn test_err_syscall_string() { #[test] fn test_syscall_string() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov64 r2, 0x5 syscall bpf_syscall_string mov64 r0, 0x0 exit", + config, [72, 101, 108, 108, 111], ( "bpf_syscall_string" => syscalls::SyscallString::vm, @@ -2593,6 +2618,10 @@ fn test_syscall_string() { #[test] fn test_syscall() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov64 r1, 0xAA @@ -2603,6 +2632,7 @@ fn test_syscall() { syscall bpf_syscall_u64 mov64 r0, 0x0 exit", + config, [], ( "bpf_syscall_u64" => syscalls::SyscallU64::vm, @@ -2614,6 +2644,10 @@ fn test_syscall() { #[test] fn test_call_gather_bytes() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov r1, 1 @@ -2623,6 +2657,7 @@ fn test_call_gather_bytes() { mov r5, 5 syscall bpf_gather_bytes exit", + config, [], ( "bpf_gather_bytes" => syscalls::SyscallGatherBytes::vm, @@ -2634,6 +2669,10 @@ fn test_call_gather_bytes() { #[test] fn test_call_memfrob() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov r6, r1 @@ -2643,6 +2682,7 @@ fn test_call_memfrob() { ldxdw r0, [r6] be64 r0 exit", + config, [ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, // ], @@ -2684,7 +2724,11 @@ declare_builtin_function!( function_registry .register_function_hashed(*b"nested_vm_syscall", SyscallNestedVm::vm) .unwrap(); - let loader = BuiltinProgram::new_loader(Config::default(), function_registry); + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; + let loader = BuiltinProgram::new_loader(config, function_registry); let mem = [depth as u8 - 1, throw as u8]; let mut executable = assemble::( " @@ -2780,12 +2824,17 @@ fn test_tight_infinite_recursion_callx() { #[test] fn test_instruction_count_syscall() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov64 r2, 0x5 syscall bpf_syscall_string mov64 r0, 0x0 exit", + config, [72, 101, 108, 108, 111], ( "bpf_syscall_string" => syscalls::SyscallString::vm, @@ -2797,12 +2846,17 @@ fn test_instruction_count_syscall() { #[test] fn test_err_instruction_count_syscall_capped() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov64 r2, 0x5 syscall bpf_syscall_string mov64 r0, 0x0 exit", + config, [72, 101, 108, 108, 111], ( "bpf_syscall_string" => syscalls::SyscallString::vm, @@ -2814,6 +2868,10 @@ fn test_err_instruction_count_syscall_capped() { #[test] fn test_err_non_terminate_capped() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov64 r6, 0x0 @@ -2826,6 +2884,7 @@ fn test_err_non_terminate_capped() { add64 r6, 0x1 ja -0x8 exit", + config.clone(), [], ( "bpf_trace_printf" => syscalls::SyscallTracePrintf::vm, @@ -2845,6 +2904,7 @@ fn test_err_non_terminate_capped() { add64 r6, 0x1 ja -0x8 exit", + config, [], ( "bpf_trace_printf" => syscalls::SyscallTracePrintf::vm, @@ -2952,6 +3012,10 @@ fn test_far_jumps() { #[test] fn test_symbol_relocation() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; test_interpreter_and_jit_asm!( " mov64 r1, r10 @@ -2960,6 +3024,7 @@ fn test_symbol_relocation() { syscall bpf_syscall_string mov64 r0, 0x0 exit", + config, [72, 101, 108, 108, 111], ( "bpf_syscall_string" => syscalls::SyscallString::vm, diff --git a/tests/verifier.rs b/tests/verifier.rs index f0f54635..61c74899 100644 --- a/tests/verifier.rs +++ b/tests/verifier.rs @@ -367,19 +367,17 @@ fn test_verifier_unknown_sycall() { #[test] fn test_verifier_known_syscall() { let prog = &[ - 0x85, 0x00, 0x00, 0x00, 0xfe, 0xc3, 0xf5, 0x6b, // call 0x6bf5c3fe + 0x95, 0x00, 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, // syscall 2 0x9d, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // return ]; - let mut function_registry = FunctionRegistry::>::default(); - function_registry - .register_function(0x6bf5c3fe, b"my_syscall", syscalls::SyscallString::vm) + + let mut loader = BuiltinProgram::new_loader_with_dense_registration(Config::default()); + loader + .register_function("my_syscall", syscalls::SyscallString::vm, 2) .unwrap(); let executable = Executable::::from_text_bytes( prog, - Arc::new(BuiltinProgram::new_loader( - Config::default(), - function_registry, - )), + Arc::new(loader), SBPFVersion::V2, FunctionRegistry::default(), ) @@ -489,7 +487,7 @@ fn return_instr() { .unwrap(); let result = executable.verify::(); if sbpf_version == SBPFVersion::V2 { - assert!(result.is_ok()); + assert_error!(result, "VerifierError(InvalidSyscall(0))"); } else { assert_error!(result, "VerifierError(UnknownOpCode(157, 2))"); } From 3f9dee6d667d62ff4ec80c98c4c90941afd9717d Mon Sep 17 00:00:00 2001 From: Lucas Steuernagel Date: Fri, 18 Oct 2024 18:50:01 -0300 Subject: [PATCH 3/8] Update comment --- src/program.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/program.rs b/src/program.rs index 48041a3b..96ac8296 100644 --- a/src/program.rs +++ b/src/program.rs @@ -274,8 +274,8 @@ impl BuiltinProgram { } } - /// Create a new loader with both dense and sparse function registrations - /// Use `BuiltinProgram::register_function` to register. + /// Create a new loader with both dense and sparse function indexation + /// Use `BuiltinProgram::register_function` for registrations. pub fn new_loader_with_dense_registration(config: Config) -> Self { Self { config: Some(Box::new(config)), From 656626a1de0e05e8885294c5b7655338261c29a2 Mon Sep 17 00:00:00 2001 From: Lucas Steuernagel Date: Mon, 21 Oct 2024 15:08:05 -0300 Subject: [PATCH 4/8] Merge get_dense and get_sparse registry functions --- src/disassembler.rs | 2 +- src/elf.rs | 4 ++-- src/interpreter.rs | 2 +- src/jit.rs | 2 +- src/program.rs | 20 +++++++++++--------- src/static_analysis.rs | 11 ++++++++--- 6 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/disassembler.rs b/src/disassembler.rs index 23dc1b87..fbfe2c58 100644 --- a/src/disassembler.rs +++ b/src/disassembler.rs @@ -271,7 +271,7 @@ pub fn disassemble_instruction( function_name } else { name = "syscall"; - loader.get_sparse_function_registry().lookup_by_key(insn.imm as u32).map(|(function_name, _)| String::from_utf8_lossy(function_name).to_string()).unwrap_or_else(|| "[invalid]".to_string()) + loader.get_function_registry(sbpf_version).lookup_by_key(insn.imm as u32).map(|(function_name, _)| String::from_utf8_lossy(function_name).to_string()).unwrap_or_else(|| "[invalid]".to_string()) }; desc = format!("{name} {function_name}"); }, diff --git a/src/elf.rs b/src/elf.rs index 6250d778..1b39b3a6 100644 --- a/src/elf.rs +++ b/src/elf.rs @@ -315,7 +315,7 @@ impl Executable { self.get_config(), self.get_sbpf_version(), self.get_function_registry(), - self.loader.get_dense_function_registry(), + self.loader.get_function_registry(self.get_sbpf_version()), )?; Ok(()) } @@ -1072,7 +1072,7 @@ impl Executable { .or_insert_with(|| ebpf::hash_symbol_name(name)); if config.reject_broken_elfs && loader - .get_sparse_function_registry() + .get_function_registry(&SBPFVersion::V1) .lookup_by_key(hash) .is_none() { diff --git a/src/interpreter.rs b/src/interpreter.rs index 26e8c800..9a257608 100644 --- a/src/interpreter.rs +++ b/src/interpreter.rs @@ -536,7 +536,7 @@ impl<'a, 'b, C: ContextObject> Interpreter<'a, 'b, C> { }; if external { - if let Some((_function_name, function)) = self.executable.get_loader().get_sparse_function_registry().lookup_by_key(insn.imm as u32) { + if let Some((_function_name, function)) = self.executable.get_loader().get_function_registry(self.executable.get_sbpf_version()).lookup_by_key(insn.imm as u32) { resolved = true; self.vm.due_insn_count = self.vm.previous_instruction_meter - self.vm.due_insn_count; diff --git a/src/jit.rs b/src/jit.rs index c961828f..d0a17180 100644 --- a/src/jit.rs +++ b/src/jit.rs @@ -717,7 +717,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> { }; if external { - if let Some((_function_name, function)) = self.executable.get_loader().get_sparse_function_registry().lookup_by_key(insn.imm as u32) { + if let Some((_function_name, function)) = self.executable.get_loader().get_function_registry(self.executable.get_sbpf_version()).lookup_by_key(insn.imm as u32) { self.emit_validate_and_profile_instruction_count(false, Some(0)); self.emit_ins(X86Instruction::load_immediate(OperandSize::S64, REGISTER_SCRATCH, function as usize as i64)); self.emit_ins(X86Instruction::call_immediate(self.relative_to_anchor(ANCHOR_EXTERNAL_FUNCTION_CALL, 5))); diff --git a/src/program.rs b/src/program.rs index 96ac8296..83004484 100644 --- a/src/program.rs +++ b/src/program.rs @@ -154,7 +154,7 @@ impl FunctionRegistry { ebpf::hash_symbol_name(&usize::from(value).to_le_bytes()) }; if loader - .get_sparse_function_registry() + .get_function_registry(&SBPFVersion::V1) .lookup_by_key(hash) .is_some() { @@ -289,14 +289,16 @@ impl BuiltinProgram { self.config.as_ref().unwrap() } - /// Get the sparse function registry - pub fn get_sparse_function_registry(&self) -> &FunctionRegistry> { - &self.sparse_registry - } - - /// Get the dense function registry - pub fn get_dense_function_registry(&self) -> &FunctionRegistry> { - &self.dense_registry + /// Get the function registry depending on the SBPF version + pub fn get_function_registry( + &self, + sbpf_version: &SBPFVersion, + ) -> &FunctionRegistry> { + if sbpf_version == &SBPFVersion::V1 { + &self.sparse_registry + } else { + &self.dense_registry + } } /// Calculate memory size diff --git a/src/static_analysis.rs b/src/static_analysis.rs index ab73c6cc..2d95175d 100644 --- a/src/static_analysis.rs +++ b/src/static_analysis.rs @@ -6,6 +6,7 @@ use crate::{ ebpf, elf::Executable, error::EbpfError, + program::SBPFVersion, vm::{ContextObject, DynamicAnalysis, TestContextObject}, }; use rustc_demangle::demangle; @@ -192,7 +193,7 @@ impl<'a> Analysis<'a> { dfg_forward_edges: BTreeMap::new(), dfg_reverse_edges: BTreeMap::new(), }; - result.split_into_basic_blocks(false); + result.split_into_basic_blocks(false, executable.get_sbpf_version()); result.control_flow_graph_tarjan(); result.control_flow_graph_dominance_hierarchy(); result.label_basic_blocks(); @@ -223,7 +224,11 @@ impl<'a> Analysis<'a> { /// Splits the sequence of instructions into basic blocks /// /// Also links the control-flow graph edges between the basic blocks. - pub fn split_into_basic_blocks(&mut self, flatten_call_graph: bool) { + pub fn split_into_basic_blocks( + &mut self, + flatten_call_graph: bool, + sbpf_version: &SBPFVersion, + ) { self.cfg_nodes.insert(0, CfgNode::default()); for pc in self.functions.keys() { self.cfg_nodes.entry(*pc).or_default(); @@ -236,7 +241,7 @@ impl<'a> Analysis<'a> { if let Some((function_name, _function)) = self .executable .get_loader() - .get_sparse_function_registry() + .get_function_registry(sbpf_version) .lookup_by_key(insn.imm as u32) { if function_name == b"abort" { From 2f44c653920ea85cf61e2f3277396ec5a32e624e Mon Sep 17 00:00:00 2001 From: Lucas Steuernagel Date: Mon, 21 Oct 2024 15:19:02 -0300 Subject: [PATCH 5/8] Remove syscall from test --- tests/execution.rs | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/tests/execution.rs b/tests/execution.rs index f9fbe4be..54c1be01 100644 --- a/tests/execution.rs +++ b/tests/execution.rs @@ -2868,10 +2868,6 @@ fn test_err_instruction_count_syscall_capped() { #[test] fn test_err_non_terminate_capped() { - let config = Config { - enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, - ..Config::default() - }; test_interpreter_and_jit_asm!( " mov64 r6, 0x0 @@ -2880,15 +2876,11 @@ fn test_err_non_terminate_capped() { mov64 r3, 0x0 mov64 r4, 0x0 mov64 r5, r6 - syscall bpf_trace_printf add64 r6, 0x1 ja -0x8 exit", - config.clone(), [], - ( - "bpf_trace_printf" => syscalls::SyscallTracePrintf::vm, - ), + (), TestContextObject::new(7), ProgramResult::Err(EbpfError::ExceededMaxInstructions), ); @@ -2900,15 +2892,11 @@ fn test_err_non_terminate_capped() { mov64 r3, 0x0 mov64 r4, 0x0 mov64 r5, r6 - syscall bpf_trace_printf add64 r6, 0x1 ja -0x8 exit", - config, [], - ( - "bpf_trace_printf" => syscalls::SyscallTracePrintf::vm, - ), + (), TestContextObject::new(1000), ProgramResult::Err(EbpfError::ExceededMaxInstructions), ); From 32f43bb7f0cb232d8fcdf781ae49c4bb5a347f67 Mon Sep 17 00:00:00 2001 From: Lucas Steuernagel Date: Mon, 21 Oct 2024 15:29:06 -0300 Subject: [PATCH 6/8] Rebase onto main --- src/elf.rs | 2 +- src/program.rs | 6 +++--- src/static_analysis.rs | 6 +----- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/elf.rs b/src/elf.rs index 1b39b3a6..640073c3 100644 --- a/src/elf.rs +++ b/src/elf.rs @@ -1072,7 +1072,7 @@ impl Executable { .or_insert_with(|| ebpf::hash_symbol_name(name)); if config.reject_broken_elfs && loader - .get_function_registry(&SBPFVersion::V1) + .get_function_registry(SBPFVersion::V1) .lookup_by_key(hash) .is_none() { diff --git a/src/program.rs b/src/program.rs index 83004484..781849cc 100644 --- a/src/program.rs +++ b/src/program.rs @@ -154,7 +154,7 @@ impl FunctionRegistry { ebpf::hash_symbol_name(&usize::from(value).to_le_bytes()) }; if loader - .get_function_registry(&SBPFVersion::V1) + .get_function_registry(SBPFVersion::V1) .lookup_by_key(hash) .is_some() { @@ -292,9 +292,9 @@ impl BuiltinProgram { /// Get the function registry depending on the SBPF version pub fn get_function_registry( &self, - sbpf_version: &SBPFVersion, + sbpf_version: SBPFVersion, ) -> &FunctionRegistry> { - if sbpf_version == &SBPFVersion::V1 { + if sbpf_version == SBPFVersion::V1 { &self.sparse_registry } else { &self.dense_registry diff --git a/src/static_analysis.rs b/src/static_analysis.rs index 2d95175d..77dde3b1 100644 --- a/src/static_analysis.rs +++ b/src/static_analysis.rs @@ -224,11 +224,7 @@ impl<'a> Analysis<'a> { /// Splits the sequence of instructions into basic blocks /// /// Also links the control-flow graph edges between the basic blocks. - pub fn split_into_basic_blocks( - &mut self, - flatten_call_graph: bool, - sbpf_version: &SBPFVersion, - ) { + pub fn split_into_basic_blocks(&mut self, flatten_call_graph: bool, sbpf_version: SBPFVersion) { self.cfg_nodes.insert(0, CfgNode::default()); for pc in self.functions.keys() { self.cfg_nodes.entry(*pc).or_default(); From 0dca84981c60da4ff27b7ba7f9f7080ef2a260ac Mon Sep 17 00:00:00 2001 From: Lucas Steuernagel Date: Mon, 21 Oct 2024 18:40:43 -0300 Subject: [PATCH 7/8] Check for static syscall instead of SBPFv1 --- src/program.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/program.rs b/src/program.rs index 781849cc..0636f64f 100644 --- a/src/program.rs +++ b/src/program.rs @@ -294,10 +294,10 @@ impl BuiltinProgram { &self, sbpf_version: SBPFVersion, ) -> &FunctionRegistry> { - if sbpf_version == SBPFVersion::V1 { - &self.sparse_registry - } else { + if sbpf_version.static_syscalls() { &self.dense_registry + } else { + &self.sparse_registry } } From dcedab8879b0d8e3be7ac2209e0897ca42d36e9e Mon Sep 17 00:00:00 2001 From: Lucas Steuernagel Date: Mon, 21 Oct 2024 18:53:32 -0300 Subject: [PATCH 8/8] Swap parameters --- src/program.rs | 2 +- tests/verifier.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/program.rs b/src/program.rs index 0636f64f..e40998a5 100644 --- a/src/program.rs +++ b/src/program.rs @@ -317,8 +317,8 @@ impl BuiltinProgram { pub fn register_function( &mut self, name: &str, - value: BuiltinFunction, dense_key: u32, + value: BuiltinFunction, ) -> Result<(), ElfError> { self.sparse_registry.register_function_hashed(name, value)?; self.dense_registry diff --git a/tests/verifier.rs b/tests/verifier.rs index 61c74899..dc846c58 100644 --- a/tests/verifier.rs +++ b/tests/verifier.rs @@ -373,7 +373,7 @@ fn test_verifier_known_syscall() { let mut loader = BuiltinProgram::new_loader_with_dense_registration(Config::default()); loader - .register_function("my_syscall", syscalls::SyscallString::vm, 2) + .register_function("my_syscall", 2, syscalls::SyscallString::vm) .unwrap(); let executable = Executable::::from_text_bytes( prog,