Check if syscall is valid during verification (#585)

solana-labs · Sep 6, 2024 · 9f8d7cb · 9f8d7cb
1 parent 9cc5dfc
commit 9f8d7cb
Show file tree

Hide file tree

Showing 9 changed files with 110 additions and 23 deletions.
diff --git 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::<BuiltinFunction<TestContextObject>>::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
@@ -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::<BuiltinFunction<TestContextObject>>::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
@@ -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::<BuiltinFunction<TestContextObject>>::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
@@ -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::<BuiltinFunction<TestContextObject>>::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
@@ -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::<BuiltinFunction<TestContextObject>>::default();
+
     RequisiteVerifier::verify(
         prog.into_bytes(),
         &config,
         &SBPFVersion::V2,
         &function_registry,
+        &syscall_registry,
     )
     .unwrap();
 });
diff --git a/src/elf.rs b/src/elf.rs
@@ -315,6 +315,7 @@ impl<C: ContextObject> Executable<C> {
             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
@@ -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<C: ContextObject>(
         prog: &[u8],
         config: &Config,
         sbpf_version: &SBPFVersion,
         function_registry: &FunctionRegistry<usize>,
+        syscall_registry: &FunctionRegistry<BuiltinFunction<C>>,
     ) -> Result<(), VerifierError>;
 }
 
@@ -153,10 +154,14 @@ fn check_jmp_offset(
     Ok(())
 }
 
-fn check_call_target(
+fn check_call_target<T>(
     key: u32,
-    function_registry: &FunctionRegistry<usize>,
-) -> Result<(), VerifierError> {
+    function_registry: &FunctionRegistry<T>,
+) -> 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<usize>) -> Result<(), VerifierError> {
+    fn verify<C: ContextObject>(prog: &[u8], config: &Config, sbpf_version: &SBPFVersion, function_registry: &FunctionRegistry<usize>, syscall_registry: &FunctionRegistry<BuiltinFunction<C>>) -> 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
@@ -41,14 +41,19 @@ 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;
         let expected_result = format!("{:?}", $expected_result);
         if !expected_result.contains("ExceededMaxInstructions") {
             context_object.remaining = INSTRUCTION_METER_BUDGET;
         }
-        $executable.verify::<RequisiteVerifier>().unwrap();
+        if $verify {
+            $executable.verify::<RequisiteVerifier>().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::<TestContextObject>::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
@@ -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,23 +44,25 @@ pub enum VerifierTestError {
 
 struct TautologyVerifier {}
 impl Verifier for TautologyVerifier {
-    fn verify(
+    fn verify<C: ContextObject>(
         _prog: &[u8],
         _config: &Config,
         _sbpf_version: &SBPFVersion,
         _function_registry: &FunctionRegistry<usize>,
+        _syscall_registry: &FunctionRegistry<BuiltinFunction<C>>,
     ) -> std::result::Result<(), VerifierError> {
         Ok(())
     }
 }
 
 struct ContradictionVerifier {}
 impl Verifier for ContradictionVerifier {
-    fn verify(
+    fn verify<C: ContextObject>(
         _prog: &[u8],
         _config: &Config,
         _sbpf_version: &SBPFVersion,
         _function_registry: &FunctionRegistry<usize>,
+        _syscall_registry: &FunctionRegistry<BuiltinFunction<C>>,
     ) -> std::result::Result<(), VerifierError> {
         Err(VerifierError::NoProgram)
     }
@@ -299,6 +302,46 @@ fn test_verifier_err_unknown_opcode() {
     executable.verify::<RequisiteVerifier>().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::<TestContextObject>::from_text_bytes(
+        prog,
+        Arc::new(BuiltinProgram::new_mock()),
+        SBPFVersion::V2,
+        FunctionRegistry::default(),
+    )
+    .unwrap();
+    executable.verify::<RequisiteVerifier>().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::<BuiltinFunction<TestContextObject>>::default();
+    function_registry
+        .register_function(0x6bf5c3fe, b"my_syscall", syscalls::SyscallString::vm)
+        .unwrap();
+    let executable = Executable::<TestContextObject>::from_text_bytes(
+        prog,
+        Arc::new(BuiltinProgram::new_loader(
+            Config::default(),
+            function_registry,
+        )),
+        SBPFVersion::V2,
+        FunctionRegistry::default(),
+    )
+    .unwrap();
+    executable.verify::<RequisiteVerifier>().unwrap();
+}
+
 #[test]
 #[should_panic(expected = "CannotWriteR10(0)")]
 fn test_verifier_err_write_r10() {