From 9f8d7cb694c8afd28f6e42d38826036c77ea7b52 Mon Sep 17 00:00:00 2001 From: Lucas Ste <38472950+LucasSte@users.noreply.github.com> Date: Fri, 6 Sep 2024 16:56:57 -0300 Subject: [PATCH] Check if syscall is valid during verification (#585) --- fuzz/fuzz_targets/dumb.rs | 6 ++- fuzz/fuzz_targets/smart.rs | 5 ++- fuzz/fuzz_targets/smart_jit_diff.rs | 5 ++- fuzz/fuzz_targets/smarter_jit_diff.rs | 5 ++- fuzz/fuzz_targets/verify_semantic_aware.rs | 6 ++- src/elf.rs | 1 + src/verifier.rs | 20 ++++++--- tests/execution.rs | 34 ++++++++++++--- tests/verifier.rs | 51 ++++++++++++++++++++-- 9 files changed, 110 insertions(+), 23 deletions(-) diff --git a/fuzz/fuzz_targets/dumb.rs b/fuzz/fuzz_targets/dumb.rs index e8b17c2b..c1b1743d 100644 --- a/fuzz/fuzz_targets/dumb.rs +++ b/fuzz/fuzz_targets/dumb.rs @@ -8,7 +8,7 @@ use solana_rbpf::{ ebpf, elf::Executable, memory_region::MemoryRegion, - program::{BuiltinProgram, FunctionRegistry, SBPFVersion}, + program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SBPFVersion}, verifier::{RequisiteVerifier, Verifier}, vm::TestContextObject, }; @@ -29,7 +29,9 @@ fuzz_target!(|data: DumbFuzzData| { let prog = data.prog; let config = data.template.into(); let function_registry = FunctionRegistry::default(); - if RequisiteVerifier::verify(&prog, &config, &SBPFVersion::V2, &function_registry).is_err() { + let syscall_registry = FunctionRegistry::>::default(); + + if RequisiteVerifier::verify(&prog, &config, &SBPFVersion::V2, &function_registry, &syscall_registry).is_err() { // verify please return; } diff --git a/fuzz/fuzz_targets/smart.rs b/fuzz/fuzz_targets/smart.rs index 8ccb873e..0ecb535b 100644 --- a/fuzz/fuzz_targets/smart.rs +++ b/fuzz/fuzz_targets/smart.rs @@ -10,7 +10,7 @@ use solana_rbpf::{ elf::Executable, insn_builder::{Arch, IntoBytes}, memory_region::MemoryRegion, - program::{BuiltinProgram, FunctionRegistry, SBPFVersion}, + program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SBPFVersion}, verifier::{RequisiteVerifier, Verifier}, vm::TestContextObject, }; @@ -33,11 +33,14 @@ fuzz_target!(|data: FuzzData| { let prog = make_program(&data.prog, data.arch); let config = data.template.into(); let function_registry = FunctionRegistry::default(); + let syscall_registry = FunctionRegistry::>::default(); + if RequisiteVerifier::verify( prog.into_bytes(), &config, &SBPFVersion::V2, &function_registry, + &syscall_registry, ) .is_err() { diff --git a/fuzz/fuzz_targets/smart_jit_diff.rs b/fuzz/fuzz_targets/smart_jit_diff.rs index d085a0d0..87203ca2 100644 --- a/fuzz/fuzz_targets/smart_jit_diff.rs +++ b/fuzz/fuzz_targets/smart_jit_diff.rs @@ -8,7 +8,7 @@ use solana_rbpf::{ elf::Executable, insn_builder::{Arch, Instruction, IntoBytes}, memory_region::MemoryRegion, - program::{BuiltinProgram, FunctionRegistry, SBPFVersion}, + program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SBPFVersion}, verifier::{RequisiteVerifier, Verifier}, vm::TestContextObject, }; @@ -40,11 +40,14 @@ fuzz_target!(|data: FuzzData| { .push(); let config = data.template.into(); let function_registry = FunctionRegistry::default(); + let syscall_registry = FunctionRegistry::>::default(); + if RequisiteVerifier::verify( prog.into_bytes(), &config, &SBPFVersion::V2, &function_registry, + &syscall_registry, ) .is_err() { diff --git a/fuzz/fuzz_targets/smarter_jit_diff.rs b/fuzz/fuzz_targets/smarter_jit_diff.rs index 476ba689..6d77df41 100644 --- a/fuzz/fuzz_targets/smarter_jit_diff.rs +++ b/fuzz/fuzz_targets/smarter_jit_diff.rs @@ -8,7 +8,7 @@ use solana_rbpf::{ elf::Executable, insn_builder::IntoBytes, memory_region::MemoryRegion, - program::{BuiltinProgram, FunctionRegistry, SBPFVersion}, + program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SBPFVersion}, verifier::{RequisiteVerifier, Verifier}, vm::TestContextObject, }; @@ -30,11 +30,14 @@ fuzz_target!(|data: FuzzData| { let prog = make_program(&data.prog); let config = data.template.into(); let function_registry = FunctionRegistry::default(); + let syscall_registry = FunctionRegistry::>::default(); + if RequisiteVerifier::verify( prog.into_bytes(), &config, &SBPFVersion::V2, &function_registry, + &syscall_registry, ) .is_err() { diff --git a/fuzz/fuzz_targets/verify_semantic_aware.rs b/fuzz/fuzz_targets/verify_semantic_aware.rs index c1e4e171..0c2cc09f 100644 --- a/fuzz/fuzz_targets/verify_semantic_aware.rs +++ b/fuzz/fuzz_targets/verify_semantic_aware.rs @@ -5,8 +5,9 @@ use libfuzzer_sys::fuzz_target; use semantic_aware::*; use solana_rbpf::{ insn_builder::IntoBytes, - program::{FunctionRegistry, SBPFVersion}, + program::{BuiltinFunction, FunctionRegistry, SBPFVersion}, verifier::{RequisiteVerifier, Verifier}, + vm::TestContextObject, }; use crate::common::ConfigTemplate; @@ -24,11 +25,14 @@ fuzz_target!(|data: FuzzData| { let prog = make_program(&data.prog); let config = data.template.into(); let function_registry = FunctionRegistry::default(); + let syscall_registry = FunctionRegistry::>::default(); + RequisiteVerifier::verify( prog.into_bytes(), &config, &SBPFVersion::V2, &function_registry, + &syscall_registry, ) .unwrap(); }); diff --git a/src/elf.rs b/src/elf.rs index c5b531d4..9a16affa 100644 --- a/src/elf.rs +++ b/src/elf.rs @@ -315,6 +315,7 @@ impl Executable { self.get_config(), self.get_sbpf_version(), self.get_function_registry(), + self.loader.get_function_registry(), )?; Ok(()) } diff --git a/src/verifier.rs b/src/verifier.rs index c4f774ba..1846f0de 100644 --- a/src/verifier.rs +++ b/src/verifier.rs @@ -14,8 +14,8 @@ use crate::{ ebpf, - program::{FunctionRegistry, SBPFVersion}, - vm::Config, + program::{BuiltinFunction, FunctionRegistry, SBPFVersion}, + vm::{Config, ContextObject}, }; use thiserror::Error; @@ -85,11 +85,12 @@ pub trait Verifier { /// - Unknown instructions. /// - Bad formed instruction. /// - Unknown eBPF syscall index. - fn verify( + fn verify( prog: &[u8], config: &Config, sbpf_version: &SBPFVersion, function_registry: &FunctionRegistry, + syscall_registry: &FunctionRegistry>, ) -> Result<(), VerifierError>; } @@ -153,10 +154,14 @@ fn check_jmp_offset( Ok(()) } -fn check_call_target( +fn check_call_target( key: u32, - function_registry: &FunctionRegistry, -) -> Result<(), VerifierError> { + function_registry: &FunctionRegistry, +) -> Result<(), VerifierError> +where + T: Copy, + T: PartialEq, +{ function_registry .lookup_by_key(key) .map(|_| ()) @@ -216,7 +221,7 @@ pub struct RequisiteVerifier {} impl Verifier for RequisiteVerifier { /// Check the program against the verifier's rules #[rustfmt::skip] - fn verify(prog: &[u8], config: &Config, sbpf_version: &SBPFVersion, function_registry: &FunctionRegistry) -> Result<(), VerifierError> { + fn verify(prog: &[u8], config: &Config, sbpf_version: &SBPFVersion, function_registry: &FunctionRegistry, syscall_registry: &FunctionRegistry>) -> Result<(), VerifierError> { check_prog_len(prog)?; let program_range = 0..prog.len() / ebpf::INSN_SIZE; @@ -371,6 +376,7 @@ impl Verifier for RequisiteVerifier { 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 => {}, ebpf::CALL_REG => { check_callx_register(&insn, insn_ptr, config, sbpf_version)?; }, ebpf::EXIT => {}, diff --git a/tests/execution.rs b/tests/execution.rs index 20431324..e7f77ce2 100644 --- a/tests/execution.rs +++ b/tests/execution.rs @@ -41,6 +41,9 @@ macro_rules! test_interpreter_and_jit { .unwrap(); }; ($executable:expr, $mem:tt, $context_object:expr, $expected_result:expr $(,)?) => { + test_interpreter_and_jit!(true, $executable, $mem, $context_object, $expected_result) + }; + ($verify:literal, $executable:expr, $mem:tt, $context_object:expr, $expected_result:expr $(,)?) => { let expected_instruction_count = $context_object.get_remaining(); #[allow(unused_mut)] let mut context_object = $context_object; @@ -48,7 +51,9 @@ macro_rules! test_interpreter_and_jit { if !expected_result.contains("ExceededMaxInstructions") { context_object.remaining = INSTRUCTION_METER_BUDGET; } - $executable.verify::().unwrap(); + if $verify { + $executable.verify::().unwrap(); + } let (instruction_count_interpreter, interpreter_final_pc, _tracer_interpreter) = { let mut mem = $mem; let mem_region = MemoryRegion::new_writable(&mut mem, ebpf::MM_INPUT_START); @@ -159,7 +164,7 @@ macro_rules! test_interpreter_and_jit_asm { } macro_rules! test_interpreter_and_jit_elf { - ($source:tt, $config:tt, $mem:tt, ($($location:expr => $syscall_function:expr),* $(,)?), $context_object:expr, $expected_result:expr $(,)?) => { + ($verify:literal, $source:tt, $config:tt, $mem:tt, ($($location:expr => $syscall_function:expr),* $(,)?), $context_object:expr, $expected_result:expr $(,)?) => { let mut file = File::open($source).unwrap(); let mut elf = Vec::new(); file.read_to_end(&mut elf).unwrap(); @@ -169,9 +174,12 @@ macro_rules! test_interpreter_and_jit_elf { $(test_interpreter_and_jit!(register, function_registry, $location => $syscall_function);)* let loader = Arc::new(BuiltinProgram::new_loader($config, function_registry)); let mut executable = Executable::::from_elf(&elf, loader).unwrap(); - test_interpreter_and_jit!(executable, $mem, $context_object, $expected_result); + test_interpreter_and_jit!($verify, executable, $mem, $context_object, $expected_result); } }; + ($source:tt, $config:tt, $mem:tt, ($($location:expr => $syscall_function:expr),* $(,)?), $context_object:expr, $expected_result:expr $(,)?) => { + test_interpreter_and_jit_elf!(true, $source, $config, $mem, ($($location => $syscall_function),*), $context_object, $expected_result); + }; ($source:tt, $mem:tt, ($($location:expr => $syscall_function:expr),* $(,)?), $context_object:expr, $expected_result:expr $(,)?) => { let config = Config { enable_instruction_tracing: true, @@ -2692,14 +2700,14 @@ fn test_non_terminate_early() { mov64 r3, 0x0 mov64 r4, 0x0 mov64 r5, r6 - syscall Unresolved + callx r6 add64 r6, 0x1 ja -0x8 exit", [], (), TestContextObject::new(7), - ProgramResult::Err(EbpfError::UnsupportedInstruction), + ProgramResult::Err(EbpfError::CallOutsideTextSegment), ); } @@ -2761,13 +2769,14 @@ fn test_err_capped_before_exception() { TestContextObject::new(4), ProgramResult::Err(EbpfError::ExceededMaxInstructions), ); + test_interpreter_and_jit_asm!( " mov64 r1, 0x0 mov64 r2, 0x0 add64 r0, 0x0 add64 r0, 0x0 - syscall Unresolved + callx r2 add64 r0, 0x0 exit", [], @@ -2870,6 +2879,11 @@ fn test_symbol_relocation() { #[test] fn test_err_call_unresolved() { + let config = Config { + enabled_sbpf_versions: SBPFVersion::V1..=SBPFVersion::V1, + ..Config::default() + }; + test_interpreter_and_jit_asm!( " mov r1, 1 @@ -2880,6 +2894,7 @@ fn test_err_call_unresolved() { syscall Unresolved mov64 r0, 0x0 exit", + config.clone(), [], (), TestContextObject::new(6), @@ -2933,8 +2948,15 @@ fn test_err_unresolved_syscall_reloc_64_32() { #[test] fn test_err_unresolved_syscall_static() { + let config = Config { + enable_instruction_tracing: true, + ..Config::default() + }; + // This case only works if we skip verification. test_interpreter_and_jit_elf!( + false, "tests/elfs/syscall_static.so", + config, [], (), TestContextObject::new(4), diff --git a/tests/verifier.rs b/tests/verifier.rs index b7fb9c23..5124e8b8 100644 --- a/tests/verifier.rs +++ b/tests/verifier.rs @@ -26,9 +26,10 @@ use solana_rbpf::{ assembler::assemble, ebpf, elf::Executable, - program::{BuiltinProgram, FunctionRegistry, SBPFVersion}, + program::{BuiltinFunction, BuiltinProgram, FunctionRegistry, SBPFVersion}, + syscalls, verifier::{RequisiteVerifier, Verifier, VerifierError}, - vm::{Config, TestContextObject}, + vm::{Config, ContextObject, TestContextObject}, }; use std::sync::Arc; use test_utils::{assert_error, create_vm}; @@ -43,11 +44,12 @@ pub enum VerifierTestError { struct TautologyVerifier {} impl Verifier for TautologyVerifier { - fn verify( + fn verify( _prog: &[u8], _config: &Config, _sbpf_version: &SBPFVersion, _function_registry: &FunctionRegistry, + _syscall_registry: &FunctionRegistry>, ) -> std::result::Result<(), VerifierError> { Ok(()) } @@ -55,11 +57,12 @@ impl Verifier for TautologyVerifier { struct ContradictionVerifier {} impl Verifier for ContradictionVerifier { - fn verify( + fn verify( _prog: &[u8], _config: &Config, _sbpf_version: &SBPFVersion, _function_registry: &FunctionRegistry, + _syscall_registry: &FunctionRegistry>, ) -> std::result::Result<(), VerifierError> { Err(VerifierError::NoProgram) } @@ -299,6 +302,46 @@ fn test_verifier_err_unknown_opcode() { executable.verify::().unwrap(); } +#[test] +#[should_panic(expected = "InvalidFunction(1811268606)")] +fn test_verifier_unknown_sycall() { + let prog = &[ + 0x85, 0x00, 0x00, 0x00, 0xfe, 0xc3, 0xf5, 0x6b, // call 0x6bf5c3fe + 0x95, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // exit + ]; + let executable = Executable::::from_text_bytes( + prog, + Arc::new(BuiltinProgram::new_mock()), + SBPFVersion::V2, + FunctionRegistry::default(), + ) + .unwrap(); + executable.verify::().unwrap(); +} + +#[test] +fn test_verifier_known_syscall() { + let prog = &[ + 0x85, 0x00, 0x00, 0x00, 0xfe, 0xc3, 0xf5, 0x6b, // call 0x6bf5c3fe + 0x95, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // exit + ]; + let mut function_registry = FunctionRegistry::>::default(); + function_registry + .register_function(0x6bf5c3fe, b"my_syscall", syscalls::SyscallString::vm) + .unwrap(); + let executable = Executable::::from_text_bytes( + prog, + Arc::new(BuiltinProgram::new_loader( + Config::default(), + function_registry, + )), + SBPFVersion::V2, + FunctionRegistry::default(), + ) + .unwrap(); + executable.verify::().unwrap(); +} + #[test] #[should_panic(expected = "CannotWriteR10(0)")] fn test_verifier_err_write_r10() {