bpf: improve bpf_probe_read_kernel_str_bytes and bpf_probe_read_user_str_bytes

This change does a few things:

- it fixes a bug in the wrappers, where we were expecting the kernel to
  return len=1 for b"\0" where it instead returns 0 and doesn't write
  out the NULL terminator

- it makes the helpers more robust by hardcoding bound checks in
  assembly so that LLVM optimizations can't transform the checks in a
  way that the verifier can't understand.

- it adds integration tests

bpf/aya-bpf/src/helpers.rs

@@ -13,7 +13,10 @@ pub use aya_bpf_bindings::helpers as gen;
 #[doc(hidden)]
 pub use gen::*;
 
-use crate::cty::{c_char, c_long, c_void};
+use crate::{
+    check_bounds_signed,
+    cty::{c_char, c_long, c_void},
+};
 
 /// Read bytes stored at `src` and store them as a `T`.
 ///
@@ -419,19 +422,23 @@ pub unsafe fn bpf_probe_read_user_str_bytes(
         dest.len() as u32,
         src as *const c_void,
     );
-    if len <= 0 {
-        return Err(-1);
-    }
 
-    let len = len as usize;
-    if len >= dest.len() {
-        // this can never happen, it's needed to tell the verifier that len is
-        // bounded
+    read_str_bytes(len, dest)
+}
+
+fn read_str_bytes(len: i64, dest: &mut [u8]) -> Result<&[u8], c_long> {
+    // The lower bound is 0, since it's what is returned for b"\0". See the
+    // bpf_probe_read_user_[user|kernel]_bytes_empty integration tests.  The upper bound
+    // check is not needed since the helper truncates, but the verifier doesn't
+    // know that so we show it the upper bound.
+    if !check_bounds_signed(len, 0, dest.len() as i64) {
         return Err(-1);
     }
 
-    // len includes NULL byte
-    Ok(&dest[..len - 1])
+    // len includes the NULL terminator but not for b"\0" for which the kernel
+    // returns len=0. So we do a saturating sub and for b"\0" we return the
+    // empty slice, for all other cases we omit the terminator.
+    Ok(&dest[..(len as usize).saturating_sub(1)])
 }
 
 /// Read a null-terminated string from _kernel space_ stored at `src` into `dest`.
@@ -572,19 +579,8 @@ pub unsafe fn bpf_probe_read_kernel_str_bytes(
         dest.len() as u32,
         src as *const c_void,
     );
-    if len <= 0 {
-        return Err(-1);
-    }
-
-    let len = len as usize;
-    if len >= dest.len() {
-        // this can never happen, it's needed to tell the verifier that len is
-        // bounded
-        return Err(-1);
-    }
 
-    // len includes NULL byte
-    Ok(&dest[..len - 1])
+    read_str_bytes(len, dest)
 }
 
 /// Write bytes to the _user space_ pointer `src` and store them as a `T`.

bpf/aya-bpf/src/lib.rs

@@ -4,12 +4,12 @@
 //!
 //! Aya-bpf is an eBPF library built with a focus on operability and developer experience.
 //! It is the kernel-space counterpart of [Aya](https://docs.rs/aya)
-
 #![doc(
     html_logo_url = "https://aya-rs.dev/assets/images/crabby.svg",
     html_favicon_url = "https://aya-rs.dev/assets/images/crabby.svg"
 )]
 #![cfg_attr(unstable, feature(never_type))]
+#![cfg_attr(target_arch = "bpf", feature(asm_experimental_arch))]
 #![allow(clippy::missing_safety_doc)]
 #![no_std]
 
@@ -72,3 +72,31 @@ pub unsafe extern "C" fn memcpy(dest: *mut u8, src: *mut u8, n: usize) {
         *((dest_base + i) as *mut u8) = *((src_base + i) as *mut u8);
     }
 }
+
+/// Check if a value is within a range, using conditional forms compatible with
+/// the verifier.
+#[inline(always)]
+pub fn check_bounds_signed(value: i64, lower: i64, upper: i64) -> bool {
+    #[cfg(target_arch = "bpf")]
+    unsafe {
+        let mut in_bounds = 0u64;
+        core::arch::asm!(
+            "if {value} s< {lower} goto +2",
+            "if {value} s> {upper} goto +1",
+            "{i} = 1",
+            i = inout(reg) in_bounds,
+            lower = in(reg) lower,
+            upper = in(reg) upper,
+            value = in(reg) value,
+        );
+        in_bounds == 1
+    }
+    // We only need this for doc tests which are compiled for the host target
+    #[cfg(not(target_arch = "bpf"))]
+    {
+        let _ = value;
+        let _ = lower;
+        let _ = upper;
+        unimplemented!()
+    }
+}

test/integration-ebpf/Cargo.toml

@@ -31,3 +31,7 @@ path = "src/test.rs"
 [[bin]]
 name = "relocations"
 path = "src/relocations.rs"
+
+[[bin]]
+name = "bpf_probe_read"
+path = "src/bpf_probe_read.rs"

test/integration-ebpf/src/bpf_probe_read.rs

@@ -0,0 +1,91 @@
+#![no_std]
+#![no_main]
+
+use aya_bpf::{
+    helpers::{bpf_probe_read_kernel_str_bytes, bpf_probe_read_user_str_bytes},
+    macros::{map, uprobe},
+    maps::Array,
+    programs::ProbeContext,
+};
+
+const RESULT_BUF_LEN: usize = 1024;
+
+macro_rules! read_str_bytes {
+    ($fun:ident, $ptr:expr, $len:expr $(,)?) => {
+        let r = unsafe {
+            let Some(ptr) = RESULT.get_ptr_mut(0) else {
+                return;
+            };
+            &mut *ptr
+        };
+
+        let s = unsafe {
+            // $len comes from ctx.arg(1) so it's dynamic and the verifier
+            // doesn't see any bounds. We do $len.min(RESULT_BUF_LEN) here to
+            // ensure that the verifier can see the upper bound, or you get:
+            //
+            // 18: (79) r7 = *(u64 *)(r7 +8)         ; R7_w=scalar()
+            // [snip]
+            // 27: (bf) r2 = r7                      ;
+            // R2_w=scalar(id=2,umax=9223372036854775807,var_off=(0x0; 0x7fffffffffffffff)) [snip]
+            // 28: (85) call bpf_probe_read_user_str#114
+            // R2 unbounded memory access, use 'var &= const' or 'if (var < const)'
+            match $fun($ptr, &mut r.buf[..$len.min(RESULT_BUF_LEN)]) {
+                Ok(s) => s,
+                Err(_) => {
+                    r.did_error = 1;
+                    return;
+                }
+            }
+        };
+        r.len = s.len();
+    };
+}
+
+#[repr(C)]
+struct TestResult {
+    did_error: u64,
+    len: usize,
+    buf: [u8; RESULT_BUF_LEN],
+}
+
+#[map]
+static RESULT: Array<TestResult> = Array::with_max_entries(1, 0);
+
+#[map]
+static KERNEL_BUFFER: Array<[u8; 1024]> = Array::with_max_entries(1, 0);
+
+#[uprobe]
+pub fn test_bpf_probe_read_user_str_bytes(ctx: ProbeContext) {
+    read_str_bytes!(
+        bpf_probe_read_user_str_bytes,
+        match ctx.arg::<*const u8>(0) {
+            Some(p) => p,
+            _ => return,
+        },
+        match ctx.arg::<usize>(1) {
+            Some(p) => p,
+            _ => return,
+        },
+    );
+}
+
+#[uprobe]
+pub fn test_bpf_probe_read_kernel_str_bytes(ctx: ProbeContext) {
+    read_str_bytes!(
+        bpf_probe_read_kernel_str_bytes,
+        match KERNEL_BUFFER.get_ptr(0) {
+            Some(p) => p as *const u8,
+            _ => return,
+        },
+        match ctx.arg::<usize>(0) {
+            Some(p) => p,
+            _ => return,
+        },
+    );
+}
+
+#[panic_handler]
+fn panic(_info: &core::panic::PanicInfo) -> ! {
+    unsafe { core::hint::unreachable_unchecked() }
+}

test/integration-test/src/tests/bpf_probe_read.rs

@@ -0,0 +1,140 @@
+use std::process::exit;
+
+use aya::{
+    include_bytes_aligned,
+    maps::Array,
+    programs::{ProgramError, UProbe},
+    Bpf,
+};
+use integration_test_macros::integration_test;
+
+const RESULT_BUF_LEN: usize = 1024;
+
+#[derive(Copy, Clone)]
+#[repr(C)]
+struct TestResult {
+    did_error: u64,
+    len: usize,
+    buf: [u8; RESULT_BUF_LEN],
+}
+
+unsafe impl aya::Pod for TestResult {}
+
+#[integration_test]
+fn bpf_probe_read_user_str_bytes() {
+    let bpf = set_user_buffer(b"foo\0", RESULT_BUF_LEN);
+    assert_eq!(result_bytes(&bpf), b"foo");
+}
+
+#[integration_test]
+fn bpf_probe_read_user_str_bytes_truncate() {
+    let s = vec![b'a'; RESULT_BUF_LEN];
+    let bpf = set_user_buffer(&s, RESULT_BUF_LEN);
+    // The kernel truncates the string and the last byte is the null terminator
+    assert_eq!(result_bytes(&bpf), &s[..RESULT_BUF_LEN - 1]);
+}
+
+#[integration_test]
+fn bpf_probe_read_user_str_bytes_empty_string() {
+    let bpf = set_user_buffer(b"\0", RESULT_BUF_LEN);
+    assert_eq!(result_bytes(&bpf), b"");
+}
+
+#[integration_test]
+fn bpf_probe_read_user_str_bytes_empty_dest() {
+    let bpf = set_user_buffer(b"foo\0", 0);
+    assert_eq!(result_bytes(&bpf), b"");
+}
+
+#[integration_test]
+fn bpf_probe_read_kernel_str_bytes() {
+    let bpf = set_kernel_buffer(b"foo\0", RESULT_BUF_LEN);
+    assert_eq!(result_bytes(&bpf), b"foo");
+}
+
+#[integration_test]
+fn bpf_probe_read_kernel_str_bytes_truncate() {
+    let s = vec![b'a'; RESULT_BUF_LEN];
+    let bpf = set_kernel_buffer(&s, RESULT_BUF_LEN);
+    // The kernel truncates the string and the last byte is the null terminator
+    assert_eq!(result_bytes(&bpf), &s[..RESULT_BUF_LEN - 1]);
+}
+
+#[integration_test]
+fn bpf_probe_read_kernel_str_bytes_empty_string() {
+    let bpf = set_kernel_buffer(b"\0", RESULT_BUF_LEN);
+    assert_eq!(result_bytes(&bpf), b"");
+}
+
+#[integration_test]
+fn bpf_probe_read_kernel_str_bytes_empty_dest() {
+    let bpf = set_kernel_buffer(b"foo\0", 0);
+    assert_eq!(result_bytes(&bpf), b"");
+}
+
+fn set_user_buffer(bytes: &[u8], dest_len: usize) -> Bpf {
+    let bpf = load_and_attach_uprobe(
+        "test_bpf_probe_read_user_str_bytes",
+        "trigger_bpf_probe_read_user",
+        include_bytes_aligned!("../../../../target/bpfel-unknown-none/release/bpf_probe_read"),
+    );
+    trigger_bpf_probe_read_user(bytes.as_ptr(), dest_len);
+    bpf
+}
+
+fn set_kernel_buffer(bytes: &[u8], dest_len: usize) -> Bpf {
+    let mut bpf = load_and_attach_uprobe(
+        "test_bpf_probe_read_kernel_str_bytes",
+        "trigger_bpf_probe_read_kernel",
+        include_bytes_aligned!("../../../../target/bpfel-unknown-none/release/bpf_probe_read"),
+    );
+    set_kernel_buffer_element(&mut bpf, bytes);
+    trigger_bpf_probe_read_kernel(dest_len);
+    bpf
+}
+
+fn set_kernel_buffer_element(bpf: &mut Bpf, bytes: &[u8]) {
+    let mut bytes = bytes.to_vec();
+    bytes.resize(1024, 0xFF);
+    let bytes: [u8; 1024] = bytes.try_into().unwrap();
+    let mut m = Array::<_, [u8; 1024]>::try_from(bpf.map_mut("KERNEL_BUFFER").unwrap()).unwrap();
+    m.set(0, bytes, 0).unwrap();
+}
+
+fn result_bytes(bpf: &Bpf) -> Vec<u8> {
+    let m = Array::<_, TestResult>::try_from(bpf.map("RESULT").unwrap()).unwrap();
+    let result = m.get(&0, 0).unwrap();
+    assert!(result.did_error == 0);
+    // assert that the buffer is always null terminated
+    assert_eq!(result.buf[result.len], 0);
+    result.buf[..result.len].to_vec()
+}
+
+fn load_and_attach_uprobe(prog_name: &str, func_name: &str, bytes: &[u8]) -> Bpf {
+    let mut bpf = Bpf::load(bytes).unwrap();
+
+    let prog: &mut UProbe = bpf.program_mut(prog_name).unwrap().try_into().unwrap();
+    if let Err(ProgramError::LoadError {
+        io_error,
+        verifier_log,
+    }) = prog.load()
+    {
+        println!(
+            "Failed to load program `{prog_name}`: {io_error}. Verifier log:\n{verifier_log:#}"
+        );
+        exit(1);
+    };
+
+    prog.attach(Some(func_name), 0, "/proc/self/exe", None)
+        .unwrap();
+
+    bpf
+}
+
+#[no_mangle]
+#[inline(never)]
+pub extern "C" fn trigger_bpf_probe_read_user(_string: *const u8, _len: usize) {}
+
+#[no_mangle]
+#[inline(never)]
+pub extern "C" fn trigger_bpf_probe_read_kernel(_len: usize) {}

test/integration-test/src/tests/mod.rs

@@ -4,6 +4,7 @@ use libc::{uname, utsname};
 use regex::Regex;
 use std::{ffi::CStr, mem};
 
+pub mod bpf_probe_read;
 pub mod btf_relocations;
 pub mod elf;
 pub mod load;