From 153c1c15dc25acdc9995ed6b4ddea4d805163db7 Mon Sep 17 00:00:00 2001 From: Mahe Tardy Date: Fri, 2 Aug 2024 15:14:06 +0200 Subject: [PATCH 1/4] bpf: bump prepend_name underlying buffer size to 4096 We need this because when reading exe for large path (>256) we ended up having only part of the end (since we walk the dentry from end to start) and thus the prefix match wasn't working. We still don't need to keep the whole path, but it needs to be correct. Signed-off-by: Mahe Tardy --- bpf/lib/process.h | 7 +------ bpf/process/bpf_execve_event.c | 23 +++++++++++++++++------ bpf/process/bpf_process_event.h | 16 +++++++--------- 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/bpf/lib/process.h b/bpf/lib/process.h index 366ac7057d8..865d32a5556 100644 --- a/bpf/lib/process.h +++ b/bpf/lib/process.h @@ -273,12 +273,7 @@ struct msg_k8s { #define BINARY_PATH_MAX_LEN 256 struct heap_exe { - // because of verifier limitations, this has to be 2 * 256 bytes while 256 - // should be theoretically sufficient, and actually is, in unit tests. - char buf[BINARY_PATH_MAX_LEN * 2]; - // offset points to the start of the path in the above buffer. Use offset to - // read the path in the buffer since it's written from the end. - char *off; + char buf[BINARY_PATH_MAX_LEN]; __u32 len; __u32 error; }; // All fields aligned so no 'packed' attribute. diff --git a/bpf/process/bpf_execve_event.c b/bpf/process/bpf_execve_event.c index a3cc249434e..6cc4288f262 100644 --- a/bpf/process/bpf_execve_event.c +++ b/bpf/process/bpf_execve_event.c @@ -180,11 +180,22 @@ read_exe(struct task_struct *task, struct heap_exe *exe) struct file *file = BPF_CORE_READ(task, mm, exe_file); struct path *path = __builtin_preserve_access_index(&file->f_path); - exe->len = BINARY_PATH_MAX_LEN; - exe->off = (char *)&exe->buf; - exe->off = __d_path_local(path, exe->off, (int *)&exe->len, (int *)&exe->error); - if (exe->len > 0) - exe->len = BINARY_PATH_MAX_LEN - exe->len; + // we need to walk the complete 4096 len dentry in order to have an accurate + // matching on the prefix operators, even if we only keep a subset of that + char *buffer; + + buffer = d_path_local(path, (int *)&exe->len, (int *)&exe->error); + if (!buffer) + return 0; + + // buffer used by d_path_local can contain up to MAX_BUF_LEN i.e. 4096 we + // only keep the first 255 chars for our needs (we sacrifice one char to the + // verifier for the > 0 check) + if (exe->len > 255) + exe->len = 255; + asm volatile("%[len] &= 0xff;\n" + : [len] "+r"(exe->len)); + probe_read(exe->buf, exe->len, buffer); return exe->len; } @@ -364,7 +375,7 @@ execve_send(void *ctx) #ifdef __LARGE_BPF_PROG // read from proc exe stored at execve time if (event->exe.len <= BINARY_PATH_MAX_LEN) { - curr->bin.path_length = probe_read(curr->bin.path, event->exe.len, event->exe.off); + curr->bin.path_length = probe_read(curr->bin.path, event->exe.len, event->exe.buf); if (curr->bin.path_length == 0) curr->bin.path_length = event->exe.len; } diff --git a/bpf/process/bpf_process_event.h b/bpf/process/bpf_process_event.h index 6695e63e428..07e431045f1 100644 --- a/bpf/process/bpf_process_event.h +++ b/bpf/process/bpf_process_event.h @@ -12,14 +12,14 @@ #define ENAMETOOLONG 36 /* File name too long */ #define MATCH_BINARIES_PATH_MAX_LENGTH 256 -#define MAX_BUF_LEN 256 +#define MAX_BUF_LEN 4096 struct buffer_heap_map_value { - // Buffer is twice the needed size because of the verifier. In prepend_name - // unit tests, the verifier figures out that 255 is enough and that the - // buffer_offset will not overflow, but in the real use-case it looks like - // it's forgetting about that. - unsigned char buf[MAX_BUF_LEN * 2]; + // Buffer need a bit more space here because of the verifier. In + // prepend_name unit tests, the verifier figures out that MAX_BUF_LEN is + // enough and that the buffer_offset will not overflow, but in the real + // use-case it looks like it's forgetting about that. + unsigned char buf[MAX_BUF_LEN + 256]; }; struct { @@ -119,15 +119,13 @@ prepend_name(char *buf, char **bufptr, int *buflen, const char *name, u32 namele *buflen -= (namelen + write_slash); - // This will not happen as buffer_offset cannot be above 256 and namelen is - // bound to 255. Needed to make the verifier happy in older kernels. if (namelen + write_slash > buffer_offset) return -ENAMETOOLONG; buffer_offset -= (namelen + write_slash); // This will never happen. buffer_offset is the diff of the initial buffer pointer - // with the current buffer pointer. This will be at max 256 bytes (similar to the initial + // with the current buffer pointer. This will be at max 4096 bytes (similar to the initial // size). // Needed to bound that for probe_read call. if (buffer_offset >= MAX_BUF_LEN) From c141b0fac2f383d7b491b184158e30924f81bf86 Mon Sep 17 00:00:00 2001 From: Mahe Tardy Date: Fri, 2 Aug 2024 16:54:20 +0200 Subject: [PATCH 2/4] bpf: fix prepend_name unit tests for new buffer length Signed-off-by: Mahe Tardy --- Makefile | 4 ++- bpf/tests/prepend_name_test.c | 2 +- bpf/tests/prepend_name_test.go | 61 +++++++++++++++++++++++----------- 3 files changed, 46 insertions(+), 21 deletions(-) diff --git a/Makefile b/Makefile index 11e8a876eb0..dcc94a6df20 100644 --- a/Makefile +++ b/Makefile @@ -264,8 +264,10 @@ test: tester-progs tetragon-bpf ## Run Go tests. tester-progs: ## Compile helper programs for unit testing. $(MAKE) -C $(TESTER_PROGS_DIR) +## bpf-test: ## run BPF tests. +## bpf-test BPFGOTESTFLAGS="-v": ## run BPF tests with verbose. .PHONY: bpf-test -bpf-test: ## Run BPF tests. +bpf-test: $(MAKE) -C ./bpf run-test .PHONY: verify diff --git a/bpf/tests/prepend_name_test.c b/bpf/tests/prepend_name_test.c index c4bbf2881fb..3bec8c4e98c 100644 --- a/bpf/tests/prepend_name_test.c +++ b/bpf/tests/prepend_name_test.c @@ -13,7 +13,7 @@ char _license[] __attribute__((section("license"), used)) = "Dual BSD/GPL"; -#define TEST_MAX_BUF_LEN 256 +#define TEST_MAX_BUF_LEN 4096 #define NAME_MAX 255 struct test_prepend_name_state_map_value { diff --git a/bpf/tests/prepend_name_test.go b/bpf/tests/prepend_name_test.go index 93e69d2298f..525418a8a95 100644 --- a/bpf/tests/prepend_name_test.go +++ b/bpf/tests/prepend_name_test.go @@ -18,7 +18,7 @@ import ( const ( // those constants must be synchronized with the BPF code - MAX_BUF_LEN = 256 + MAX_BUF_LEN = 4096 NAME_MAX = 255 testPrependNameStateMapName = "test_prepend_name_state_map" programName = "test_prepend_name" @@ -232,44 +232,67 @@ func Test_PrependName(t *testing.T) { assert.Equal(t, "sr/bin/cat", state.BufferToString()) }) - SetupLongDentry := func() string { - // length is 239 - const longDentry = "pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil" + // length is 239 + const longDentry = "pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil_pizza_tomato_mozzarella_basil" + + t.Run("MaxSizeBufMedium", func(t *testing.T) { + const bufsize = 256 + state.ResetStateWithBuflen(bufsize) err = state.UpdateDentry(longDentry) assert.NoError(t, err) code := runPrependName() assert.Equal(t, 0, code) assert.Equal(t, "/"+longDentry, state.BufferToString()) - return longDentry - } - - t.Run("MaxSizeBufFull", func(t *testing.T) { - state.ResetStateWithBuflen(MAX_BUF_LEN) - - longDentry := SetupLongDentry() // length is 15, so 239 + 15 + 2 slash chars = 256 err = state.UpdateDentry("favorite_recipe") assert.NoError(t, err) - code := runPrependName() + code = runPrependName() assert.Equal(t, 0, code) assert.Equal(t, "/favorite_recipe"+"/"+longDentry, state.BufferToString()) - assert.Equal(t, MAX_BUF_LEN, len(state.BufferToString())) + assert.Equal(t, bufsize, len(state.BufferToString())) }) - t.Run("MaxSizeBufTooSmall", func(t *testing.T) { + t.Run("MaxSizeBufFull", func(t *testing.T) { + maxDentry := strings.Repeat("a", NAME_MAX) state.ResetStateWithBuflen(MAX_BUF_LEN) - longDentry := SetupLongDentry() + var expectedState string + // (len("/") + 255) * 16 = 4096 + for range 16 { + err = state.UpdateDentry(maxDentry) + assert.NoError(t, err) + code := runPrependName() + assert.Equal(t, 0, code) + expectedState += "/" + maxDentry + assert.Equal(t, expectedState, state.BufferToString()) + } + }) + + t.Run("MaxSizeBufTooSmall", func(t *testing.T) { + largeDentry := strings.Repeat("a", 240) + state.ResetStateWithBuflen(MAX_BUF_LEN) - // length is 16 with the "s" of "recipes", so 240 + 15 + 2 slash chars = 257 - err = state.UpdateDentry("favorite_recipes") + var expectedState string + // (len("/") + 240) * 16 = 3856 + for range 16 { + err = state.UpdateDentry(largeDentry) + assert.NoError(t, err) + code := runPrependName() + assert.Equal(t, 0, code) + expectedState = "/" + largeDentry + expectedState + assert.Equal(t, expectedState, state.BufferToString()) + } + // at this stage, there should be 240 chars left in the buf which leaves + // no space for the remaining root slash character + err = state.UpdateDentry(largeDentry) assert.NoError(t, err) code := runPrependName() assert.Equal(t, -int(unix.ENAMETOOLONG), code) - assert.Equal(t, "favorite_recipes"+"/"+longDentry, state.BufferToString()) - assert.Equal(t, MAX_BUF_LEN, len(state.BufferToString())) + // note that I intentionally don't add the '/' char + expectedState = largeDentry + expectedState + assert.Equal(t, expectedState, state.BufferToString()) }) t.Run("MaxSizeBufNormalUse", func(t *testing.T) { From e73a0a0fc39a609b55e6e94172cd3b9893b15402 Mon Sep 17 00:00:00 2001 From: Andrei Fedotov Date: Tue, 23 Jul 2024 19:08:47 +0300 Subject: [PATCH 3/4] pkg/sensors: add TestKprobeMatchBinariesPrefixLargePath Adding test that has Prefix operator in matchBinaries selector. The file path of the test binary (true) being executed is larger than 256 bytes: it should be around 3900 chars. Co-authored-by: Mahe Tardy Signed-off-by: Andrei Fedotov --- pkg/sensors/tracing/kprobe_test.go | 49 ++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/pkg/sensors/tracing/kprobe_test.go b/pkg/sensors/tracing/kprobe_test.go index 5ecc0d2902b..99539a3af56 100644 --- a/pkg/sensors/tracing/kprobe_test.go +++ b/pkg/sensors/tracing/kprobe_test.go @@ -3886,6 +3886,55 @@ func TestKprobeMatchBinaries(t *testing.T) { }) } +func TestKprobeMatchBinariesPrefixLargePath(t *testing.T) { + if !kernels.EnableLargeProgs() { + t.Skip() + } + + // create a large temporary directory path + tmpDir := t.TempDir() + targetBinLargePath := tmpDir + // add (255 + 1) * 15 = 3840 chars to the path + // max is 4096 and we want to leave some space for the tmpdir + others + for range 15 { + targetBinLargePath += "/" + strings.Repeat("a", unix.NAME_MAX) + } + err := os.MkdirAll(targetBinLargePath, 0755) + require.NoError(t, err) + + // copy the binary into it + targetBinLargePath += "/true" + fileExec, err := exec.LookPath("true") + require.NoError(t, err) + err = exec.Command("cp", fileExec, targetBinLargePath).Run() + require.NoError(t, err) + + var doneWG, readyWG sync.WaitGroup + defer doneWG.Wait() + + ctx, cancel := context.WithTimeout(context.Background(), tus.Conf().CmdWaitTime) + defer cancel() + + createCrdFile(t, getMatchBinariesCrd("Prefix", []string{tmpDir})) + + obs, err := observertesthelper.GetDefaultObserverWithFile(t, ctx, testConfigFile, tus.Conf().TetragonLib, observertesthelper.WithMyPid()) + if err != nil { + t.Fatalf("GetDefaultObserverWithFile error: %s", err) + } + observertesthelper.LoopEvents(ctx, t, &doneWG, &readyWG, obs) + readyWG.Wait() + + if err := exec.Command(targetBinLargePath).Run(); err != nil { + t.Fatalf("failed to run true: %s", err) + } + + checker := ec.NewUnorderedEventChecker(ec.NewProcessKprobeChecker(""). + WithProcess(ec.NewProcessChecker().WithBinary(sm.Full(targetBinLargePath))). + WithFunctionName(sm.Full("fd_install"))) + err = jsonchecker.JsonTestCheck(t, checker) + assert.NoError(t, err) +} + // matchBinariesPerfringTest checks that the matchBinaries do correctly // filter the events i.e. it checks that no other events appear. func matchBinariesPerfringTest(t *testing.T, operator string, values []string) { From b53131ab7d3e78dd3f09903645dff18e23cfcbcd Mon Sep 17 00:00:00 2001 From: Mahe Tardy Date: Wed, 7 Aug 2024 11:17:12 +0200 Subject: [PATCH 4/4] vmtests: bump LVH rhel8 kernel image to rhel8.9 A kernel bug fails to trigger the BPF program hooked on a tracepoint if the binary name passed as parameter is long enough, you can trigger it with a long path, using mtardy/pathgen for example and bpftrace: bpftrace -e 'tracepoint:sched:sched_process_exec { printf("execute\n"); }' Under rhel8.6, if the path is long enough (>3000 for example), the BPF prog will not be triggered. Signed-off-by: Mahe Tardy --- .github/workflows/vmtests.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/vmtests.yml b/.github/workflows/vmtests.yml index 83cd04b3f40..30e354c48f9 100644 --- a/.github/workflows/vmtests.yml +++ b/.github/workflows/vmtests.yml @@ -67,7 +67,7 @@ jobs: matrix: kernel: # renovate: datasource=docker depName=quay.io/lvh-images/kernel-images - - 'rhel8-20240620.102250' + - 'rhel8.9-20240806.173325' # renovate: datasource=docker depName=quay.io/lvh-images/kernel-images - 'bpf-next-20240624.013140' # renovate: datasource=docker depName=quay.io/lvh-images/kernel-images