From 8cee3f8b015cd741841b6b16950f8b4dec5a5744 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Wed, 27 Sep 2023 12:15:05 -0400 Subject: [PATCH 1/3] integration-test: distinguish between success and noop These tests previously produced the same result on certain failures as they did on success. --- test/integration-ebpf/src/bpf_probe_read.rs | 22 ++++++------------- .../src/tests/bpf_probe_read.rs | 12 +++++----- 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/test/integration-ebpf/src/bpf_probe_read.rs b/test/integration-ebpf/src/bpf_probe_read.rs index 02728767..64d56311 100644 --- a/test/integration-ebpf/src/bpf_probe_read.rs +++ b/test/integration-ebpf/src/bpf_probe_read.rs @@ -15,11 +15,11 @@ macro_rules! read_str_bytes { let Some(ptr) = RESULT.get_ptr_mut(0) else { return; }; - let TestResult { - did_error, - len, - buf, - } = unsafe { &mut *ptr }; + let dst = unsafe { ptr.as_mut() }; + let Some(TestResult { buf, len }) = dst else { + return; + }; + *len = None; // $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 @@ -35,22 +35,14 @@ macro_rules! read_str_bytes { return; }; - match unsafe { $fun($ptr, buf) } { - Ok(s) => { - *len = s.len(); - } - Err(_) => { - *did_error = 1; - } - } + *len = Some(unsafe { $fun($ptr, buf) }.map(<[_]>::len)); }; } #[repr(C)] struct TestResult { - did_error: u64, - len: usize, buf: [u8; RESULT_BUF_LEN], + len: Option>, } #[map] diff --git a/test/integration-test/src/tests/bpf_probe_read.rs b/test/integration-test/src/tests/bpf_probe_read.rs index bf91f271..4c425507 100644 --- a/test/integration-test/src/tests/bpf_probe_read.rs +++ b/test/integration-test/src/tests/bpf_probe_read.rs @@ -5,9 +5,8 @@ const RESULT_BUF_LEN: usize = 1024; #[derive(Copy, Clone)] #[repr(C)] struct TestResult { - did_error: u64, - len: usize, buf: [u8; RESULT_BUF_LEN], + len: Option>, } unsafe impl aya::Pod for TestResult {} @@ -96,11 +95,12 @@ fn set_kernel_buffer_element(bpf: &mut Bpf, bytes: &[u8]) { #[track_caller] fn result_bytes(bpf: &Bpf) -> Vec { let m = Array::<_, TestResult>::try_from(bpf.map("RESULT").unwrap()).unwrap(); - let result = m.get(&0, 0).unwrap(); - assert_eq!(result.did_error, 0); + let TestResult { buf, len } = m.get(&0, 0).unwrap(); + let len = len.unwrap(); + let len = len.unwrap(); // assert that the buffer is always null terminated - assert_eq!(result.buf[result.len], 0); - result.buf[..result.len].to_vec() + assert_eq!(buf[len], 0); + buf[..len].to_vec() } fn load_and_attach_uprobe(prog_name: &str, func_name: &str, bytes: &[u8]) -> Bpf { From ad460879ef84b25c93f77b7bfb8bf04b6d33ec08 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Wed, 27 Sep 2023 12:31:25 -0400 Subject: [PATCH 2/3] integration-test: replace macro with function There's just no need for a macro here. --- test/integration-ebpf/src/bpf_probe_read.rs | 85 ++++++++++----------- 1 file changed, 42 insertions(+), 43 deletions(-) diff --git a/test/integration-ebpf/src/bpf_probe_read.rs b/test/integration-ebpf/src/bpf_probe_read.rs index 64d56311..d86a6bba 100644 --- a/test/integration-ebpf/src/bpf_probe_read.rs +++ b/test/integration-ebpf/src/bpf_probe_read.rs @@ -10,33 +10,41 @@ use aya_bpf::{ const RESULT_BUF_LEN: usize = 1024; -macro_rules! read_str_bytes { - ($fun:ident, $ptr:expr, $len:expr $(,)?) => { - let Some(ptr) = RESULT.get_ptr_mut(0) else { - return; - }; - let dst = unsafe { ptr.as_mut() }; - let Some(TestResult { buf, len }) = dst else { - return; - }; - *len = None; - - // $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)' - let Some(buf) = buf.get_mut(..$len) else { - return; - }; +fn read_str_bytes( + fun: unsafe fn(*const u8, &mut [u8]) -> Result<&[u8], i64>, + iptr: Option<*const u8>, + ilen: Option, +) { + let Some(iptr) = iptr else { + return; + }; + let Some(ilen) = ilen else { + return; + }; + let Some(ptr) = RESULT.get_ptr_mut(0) else { + return; + }; + let dst = unsafe { ptr.as_mut() }; + let Some(TestResult { buf, len }) = dst else { + return; + }; + *len = None; - *len = Some(unsafe { $fun($ptr, buf) }.map(<[_]>::len)); + // 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)' + let Some(buf) = buf.get_mut(..ilen) else { + return; }; + + *len = Some(unsafe { fun(iptr, buf) }.map(<[_]>::len)); } #[repr(C)] @@ -53,31 +61,22 @@ static KERNEL_BUFFER: Array<[u8; RESULT_BUF_LEN]> = Array::with_max_entries(1, 0 #[uprobe] pub fn test_bpf_probe_read_user_str_bytes(ctx: ProbeContext) { - read_str_bytes!( + read_str_bytes( bpf_probe_read_user_str_bytes, - match ctx.arg::<*const u8>(0) { - Some(p) => p, - _ => return, - }, - match ctx.arg::(1) { - Some(p) => p, - _ => return, - }, + ctx.arg::<*const u8>(0), + ctx.arg::(1), ); } #[uprobe] pub fn test_bpf_probe_read_kernel_str_bytes(ctx: ProbeContext) { - read_str_bytes!( + 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::(0) { - Some(p) => p, - _ => return, - }, + KERNEL_BUFFER + .get_ptr(0) + .and_then(|ptr| unsafe { ptr.as_ref() }) + .map(|buf| buf.as_ptr()), + ctx.arg::(0), ); } From 15de14383d7f194430a1a4fc8d373d7c10f25244 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Wed, 27 Sep 2023 13:21:58 -0400 Subject: [PATCH 3/3] integration-test: build eBPF for the proper arch Prior to this change we neglected to build integration-ebpf for the correct target architecture, resulting in test failures on arm64. --- test/integration-test/build.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/integration-test/build.rs b/test/integration-test/build.rs index fec40483..1aee5fea 100644 --- a/test/integration-test/build.rs +++ b/test/integration-test/build.rs @@ -107,7 +107,7 @@ fn main() { } else if arch == "aarch64" { target_arch.push("arm64"); } else { - target_arch.push(arch); + target_arch.push(&arch); }; // NB: libbpf's documentation suggests that vmlinux.h be generated by running `bpftool btf @@ -198,6 +198,8 @@ fn main() { &target, ]); + cmd.env("CARGO_CFG_BPF_TARGET_ARCH", arch); + // Workaround to make sure that the rust-toolchain.toml is respected. for key in ["RUSTUP_TOOLCHAIN", "RUSTC"] { cmd.env_remove(key);