chore(ebpf): use core type size checks #4003

NDStrahilevitz · 2024-04-24T11:51:58Z

1. Explain what the PR does

1156108 chore(ebpf): replace sizeof with bpf type helpers

When using sizeof on kernel structs, false results may happen due to the
sizeof builtin not being designed to take into account btf relocations.

Use the libbpf helpers bpf_core_type_size and bpf_core_field_size where
relevant.

cfd849e chore(ebpf): remove get_type_size macro

Use the builtin libbpf core helper instead, which
the macro was an alias of.

2. Explain how to test it

3. Other comments

Resolve #3774

NDStrahilevitz · 2024-04-24T11:52:35Z

This breaks verifier right now :/

yanivagman

In some places we read a struct into a stack variable which was preallocated by the compiler according to the known size at compile time. I believe this may cause future issues and we should avoid that (so just leave as is for now)

yanivagman · 2024-04-25T10:58:49Z

pkg/ebpf/c/common/filesystem.h

@@ -171,7 +171,7 @@ statfunc size_t get_path_str_buf(struct path *path, buf_t *out_buf)
    }

    struct path f_path;
-    bpf_probe_read_kernel(&f_path, sizeof(struct path), path);
+    bpf_probe_read_kernel(&f_path, bpf_core_type_size(struct path), path);


I'm not sure it's a good idea to do that here.
The compiler preallocates space on the stack for f_path using the size of struct path known at compile time.
If the size of struct path is different on the user environment, it can override other other variables on the stack

Ok, seems reasonable to move back to sizeof in order to correspond with the the compiler preallocation.
However, doesn't this imply that the compiler might preallocate the wrong size?

Actually, it seems that in this function we can and should just rely on BPF_CORE_READ, and remove the probe_read on the struct itself, WDYT?

yanivagman · 2024-04-25T10:58:58Z

pkg/ebpf/c/common/filesystem.h

@@ -427,7 +427,7 @@ statfunc void fill_vfs_file_bin_args_io_data(io_data_t io_data, bin_args_t *bin_
        bin_args->iov_len = io_data.len;
        bin_args->iov_idx = 0;
        struct iovec io_vec;
-        bpf_probe_read_kernel(&io_vec, sizeof(struct iovec), &bin_args->vec[0]);
+        bpf_probe_read_kernel(&io_vec, bpf_core_type_size(struct iovec), &bin_args->vec[0]);


yanivagman · 2024-04-25T10:59:16Z

pkg/ebpf/c/common/filesystem.h

@@ -468,7 +468,11 @@ statfunc void fill_file_header(u8 header[FILE_MAGIC_HDR_SIZE], io_data_t io_data
        bpf_probe_read(header, len, io_data.ptr);
    } else {
        struct iovec io_vec;
+        // NOTE(nadav.str): can't use bpf_core_type_size in memset.
+        // Maybe it only resolves in runtime, although its a macro so it seems unlikely.
+        // Anyway, leaving it with sizeof for now.
        __builtin_memset(&io_vec, 0, sizeof(io_vec));


yanivagman · 2024-04-25T11:00:04Z

pkg/ebpf/c/common/network.h

@@ -369,7 +369,7 @@ statfunc struct sockaddr_un get_unix_sock_addr(struct unix_sock *sock)
    struct unix_address *addr = BPF_CORE_READ(sock, addr);
    int len = BPF_CORE_READ(addr, len);
    struct sockaddr_un sockaddr = {};
-    if (len <= sizeof(struct sockaddr_un)) {
+    if (len <= bpf_core_type_size(struct sockaddr_un)) {


yanivagman · 2024-04-25T11:00:49Z

pkg/ebpf/c/tracee.bpf.c

@@ -2641,7 +2644,7 @@ int BPF_KPROBE(trace_security_socket_connect)
        // Workaround for sockaddr_un struct length (issue: #1129).
        struct sockaddr_un sockaddr = {0};
        bpf_probe_read(&sockaddr, (u32) addr_len, (void *) address);
-        stsb(args_buf, (void *) &sockaddr, sizeof(struct sockaddr_un), 2);
+        stsb(args_buf, (void *) &sockaddr, bpf_core_type_size(struct sockaddr_un), 2);


yanivagman · 2024-04-25T11:01:16Z

pkg/ebpf/c/tracee.bpf.c

+        // NOTE(nadav.str): sizeof used instead of bpf_core_type_size in this case.
+        // bpf_core_type_size is evaluated at runtime. thus the verifier isn't satisfied
+        // with the boundary established below for addr_len. therefore, bpf_probe_read
+        // causes the verifier to fail.
        if (addr_len <= sizeof(struct sockaddr_un)) {
            struct sockaddr_un sockaddr = {};


yanivagman · 2024-04-25T11:01:37Z

pkg/ebpf/c/tracee.bpf.c

@@ -2871,7 +2881,8 @@ statfunc u32 send_bin_helper(void *ctx, void *prog_array, int tail_call)
            // Handle the rest of write recursively
            bin_args->start_off += bin_args->full_size;
            struct iovec io_vec;
-            bpf_probe_read(&io_vec, sizeof(struct iovec), &bin_args->vec[bin_args->iov_idx]);
+            bpf_probe_read(


yanivagman · 2024-04-25T11:01:45Z

pkg/ebpf/c/tracee.bpf.c

@@ -2958,7 +2969,8 @@ statfunc u32 send_bin_helper(void *ctx, void *prog_array, int tail_call)
        // Handle the rest of write recursively
        bin_args->start_off += bin_args->full_size;
        struct iovec io_vec;
-        bpf_probe_read(&io_vec, sizeof(struct iovec), &bin_args->vec[bin_args->iov_idx]);
+        bpf_probe_read(


NDStrahilevitz · 2024-04-30T13:03:12Z

I'm not sure it's a good idea to do that here.
The compiler preallocates space on the stack for f_path using the size of struct path known at compile time.
If the size of struct path is different on the user environment, it can override other other variables on the stack

@yanivagman it seems to me we should rather avoid stack allocations for kernel structs. That is, it seems that if sizeof of the stack allocated variable may turn out wrong across kernels, then the stack allocation size itself might be wrong. Not even sure we can rely on the stack allocation to be the maximum bound (then it wouldn't be such a problem), because we rely on a minimal vmlinux.h file.
Question is, is my assessment correct, and if so, should we make the effort in the scope of this PR?

Use the builtin libbpf core helper instead, which the macro was an alias of.

When using sizeof on kernel structs, false results may happen due to the sizeof builtin not being designed to take into account btf relocations. Use the libbpf helpers bpf_core_type_size and bpf_core_field_size where relevant. NOTE: iovec size checks were left unchanged, since they are currently using copies to stack allocated variables.

The helper previously copied the path argument into a stack allocated copy. This is an unnecessary copy, and risks copying the wrong size, since the actual kernel specific size may differ from the compiler allocated size. Instead, use CORE_READ to access the necessary fields directly from the argument.

NDStrahilevitz added kind/chore milestone/v0.21.0 labels Apr 24, 2024

NDStrahilevitz requested a review from yanivagman April 24, 2024 11:51

NDStrahilevitz self-assigned this Apr 24, 2024

github-actions bot added the area/ebpf label Apr 24, 2024

NDStrahilevitz force-pushed the core_type_size_checks branch 3 times, most recently from 96212d3 to 1156108 Compare April 24, 2024 16:11

NDStrahilevitz marked this pull request as ready for review April 24, 2024 16:11

yanivagman reviewed Apr 25, 2024

View reviewed changes

geyslan added milestone/v0.22.0 and removed milestone/v0.21.0 labels Apr 29, 2024

NDStrahilevitz force-pushed the core_type_size_checks branch 2 times, most recently from d04ba14 to 0dbe6cb Compare May 7, 2024 15:37

NDStrahilevitz added 3 commits May 21, 2024 12:07

chore(ebpf): remove get_type_size macro

3cf8833

Use the builtin libbpf core helper instead, which the macro was an alias of.

NDStrahilevitz force-pushed the core_type_size_checks branch from 0dbe6cb to 79049bf Compare May 21, 2024 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(ebpf): use core type size checks #4003

chore(ebpf): use core type size checks #4003

NDStrahilevitz commented Apr 24, 2024 •

edited

NDStrahilevitz commented Apr 24, 2024

yanivagman left a comment

yanivagman Apr 25, 2024 •

edited

NDStrahilevitz Apr 30, 2024

NDStrahilevitz Apr 30, 2024

yanivagman Apr 25, 2024

yanivagman Apr 25, 2024

yanivagman Apr 25, 2024

yanivagman Apr 25, 2024

yanivagman Apr 25, 2024

yanivagman Apr 25, 2024

yanivagman Apr 25, 2024

NDStrahilevitz commented Apr 30, 2024

chore(ebpf): use core type size checks #4003

Are you sure you want to change the base?

chore(ebpf): use core type size checks #4003

Conversation

NDStrahilevitz commented Apr 24, 2024 • edited

1. Explain what the PR does

2. Explain how to test it

3. Other comments

NDStrahilevitz commented Apr 24, 2024

yanivagman left a comment

Choose a reason for hiding this comment

yanivagman Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NDStrahilevitz commented Apr 30, 2024

NDStrahilevitz commented Apr 24, 2024 •

edited

yanivagman Apr 25, 2024 •

edited