Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracee should always rely on get_type_size() instead of sizeof() for correct CORE type sizes #3774

Open
rafaeldtinoco opened this issue Dec 19, 2023 · 0 comments · May be fixed by #4003
Open
Assignees
Labels
Milestone

Comments

@rafaeldtinoco
Copy link
Contributor

There are many places where sizeof() is used when it shouldn't. The type being measured by sizeof() has to be either a built-in type (int/long/...) or a local type (fully declared in a header). There are many places where the type being "sized" is a CO-RE (possible CO-RE) type:

EXamples:

diff --git a/pkg/ebpf/c/common/buffer.h b/pkg/ebpf/c/common/buffer.h
index 49d374272..6d32d421e 100644
--- a/pkg/ebpf/c/common/buffer.h
+++ b/pkg/ebpf/c/common/buffer.h
@@ -29,7 +29,7 @@ statfunc buf_t *get_buf(int idx)
 }
 
 // biggest elem to be saved with 'save_to_submit_buf' should be defined here:
-#define MAX_ELEMENT_SIZE sizeof(struct sockaddr_un)
+#define MAX_ELEMENT_SIZE get_type_size(struct sockaddr_un)
 
 statfunc int save_to_submit_buf(args_buffer_t *buf, void *ptr, u32 size, u8 index)
 {
@@ -319,7 +319,7 @@ statfunc int save_sockaddr_to_buf(args_buffer_t *buf, struct socket *sock, u8 in
         get_network_details_from_sock_v4(sk, &net_details, 0);
         get_local_sockaddr_in_from_network_details(&local, &net_details, family);
 
-        save_to_submit_buf(buf, (void *) &local, sizeof(struct sockaddr_in), index);
+        save_to_submit_buf(buf, (void *) &local, get_type_size(struct sockaddr_in), index);
     } else if (family == AF_INET6) {
         net_conn_v6_t net_details = {};
         struct sockaddr_in6 local;
@@ -327,11 +327,11 @@ statfunc int save_sockaddr_to_buf(args_buffer_t *buf, struct socket *sock, u8 in
         get_network_details_from_sock_v6(sk, &net_details, 0);
         get_local_sockaddr_in6_from_network_details(&local, &net_details, family);
 
-        save_to_submit_buf(buf, (void *) &local, sizeof(struct sockaddr_in6), index);
+        save_to_submit_buf(buf, (void *) &local, get_type_size(struct sockaddr_in6), index);
     } else if (family == AF_UNIX) {
         struct unix_sock *unix_sk = (struct unix_sock *) sk;
         struct sockaddr_un sockaddr = get_unix_sock_addr(unix_sk);
-        save_to_submit_buf(buf, (void *) &sockaddr, sizeof(struct sockaddr_un), index);
+        save_to_submit_buf(buf, (void *) &sockaddr, get_type_size(struct sockaddr_un), index);
     }
     return 0;
 }
@@ -397,13 +397,13 @@ statfunc int save_args_to_submit_buf(event_data_t *event, args_t *args)
                     bpf_probe_read(&family, sizeof(short), (void *) args->args[i]);
                     switch (family) {
                         case AF_UNIX:
-                            size = sizeof(struct sockaddr_un);
+                            size = get_type_size(struct sockaddr_un);
                             break;
                         case AF_INET:
-                            size = sizeof(struct sockaddr_in);
+                            size = get_type_size(struct sockaddr_in);
                             break;
                         case AF_INET6:
-                            size = sizeof(struct sockaddr_in6);
+                            size = get_type_size(struct sockaddr_in6);
                             break;
                         default:
                             size = sizeof(short);
@@ -419,7 +419,7 @@ statfunc int save_args_to_submit_buf(event_data_t *event, args_t *args)
                 rc = save_to_submit_buf(&(event->args_buf), (void *) (args->args[i]), size, index);
                 break;
             case TIMESPEC_T:
-                size = sizeof(struct __kernel_timespec);
+                size = get_type_size(struct __kernel_timespec);
                 rc = save_to_submit_buf(&(event->args_buf), (void *) (args->args[i]), size, index);
                 break;
         }
diff --git a/pkg/ebpf/c/common/filesystem.h b/pkg/ebpf/c/common/filesystem.h
index 6cd6b5b4e..43d1931ef 100644
--- a/pkg/ebpf/c/common/filesystem.h
+++ b/pkg/ebpf/c/common/filesystem.h
@@ -171,14 +171,14 @@ statfunc size_t get_path_str_buf(struct path *path, buf_t *out_buf)
     }
 
     struct path f_path;
-    bpf_probe_read(&f_path, sizeof(struct path), path);
+    bpf_probe_read(&f_path, get_type_size(struct path), path);
     char slash = '/';
     int zero = 0;
     struct dentry *dentry = f_path.dentry;
     struct vfsmount *vfsmnt = f_path.mnt;
     struct mount *mnt_parent_p;
     struct mount *mnt_p = real_mount(vfsmnt);
-    bpf_probe_read(&mnt_parent_p, sizeof(struct mount *), &mnt_p->mnt_parent);
+    bpf_probe_read(&mnt_parent_p, sizeof(void *), &mnt_p->mnt_parent);
     u32 buf_off = (MAX_PERCPU_BUFSIZE >> 1);
     struct dentry *mnt_root;
     struct dentry *d_parent;

This can lead to bad logic or verifier issues due to bad sizing of copies.

@rafaeldtinoco rafaeldtinoco added this to the v0.21.0 milestone Dec 19, 2023
@NDStrahilevitz NDStrahilevitz linked a pull request Apr 24, 2024 that will close this issue
@yanivagman yanivagman modified the milestones: v0.21.0, v0.22.0 May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants