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.

Sign up for GitHub

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

fix(driver): take the unix path directly from the kernel #2215

Merged
merged 1 commit into from
Jan 7, 2025
Merged

fix(driver): take the unix path directly from the kernel #2215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 5 additions & 37 deletions driver/bpf/filler_helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -606,11 +606,9 @@ static __always_inline int unix_socket_path(char *dest, const char *user_ptr, si
* specified length of the address structure.
*/
if(res == 1) {
dest[0] = '@';
res = bpf_probe_read_kernel_str(dest + 1,
res = bpf_probe_read_kernel_str(dest,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the modern we don't prepend the @ so it's probably better to be uniform between drivers, we can add it in the modern if we want but not sure what is the value of having the initial @

size - 1, // account for '@'
user_ptr + 1);
res++; // account for '@'
}
return res;
}
Expand Down Expand Up @@ -882,7 +880,7 @@ static __always_inline long bpf_fd_to_socktuple(struct filler_data *data,
*/
struct unix_sock *us = (struct unix_sock *)sk;
struct sock *speer = _READ(us->peer);
char *us_name;
char *us_name = NULL;

data->buf[data->state->tail_ctx.curoff & SCRATCH_SIZE_HALF] = socket_family_to_scap(family);

Expand All @@ -891,49 +889,19 @@ static __always_inline long bpf_fd_to_socktuple(struct filler_data *data,
memcpy(&data->buf[(data->state->tail_ctx.curoff + 1 + 8) & SCRATCH_SIZE_HALF],
&speer,
8);
us_name = ((struct sockaddr_un *)sock_address)->sun_path;
} else {
memcpy(&data->buf[(data->state->tail_ctx.curoff + 1) & SCRATCH_SIZE_HALF], &speer, 8);
memcpy(&data->buf[(data->state->tail_ctx.curoff + 1 + 8) & SCRATCH_SIZE_HALF], &us, 8);
bpf_getsockname(sock, peer_address, 1);
us_name = ((struct sockaddr_un *)peer_address)->sun_path;
}

/*
* Pack the data into the target buffer
*/
size = 1 + 8 + 8;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to simplify the verification we remove the fallback to the userspace struct like we do in the modern, we keep it only in the kernel module

if(!use_userdata) {
if(is_inbound) {
us_name = ((struct sockaddr_un *)sock_address)->sun_path;
} else {
bpf_getsockname(sock, peer_address, 1);
us_name = ((struct sockaddr_un *)peer_address)->sun_path;
}
} else {
/*
* Map the user-provided address to a sockaddr_in
*/
struct sockaddr_un *usrsockaddr_un = (struct sockaddr_un *)usrsockaddr;

/*
* Put a 0 at the end of struct sockaddr_un because
* the user might not have considered it in the length
*/
if(ulen == sizeof(struct sockaddr_storage))
((char *)usrsockaddr_un)[(ulen - 1) & SCRATCH_SIZE_MAX] = 0;
else
((char *)usrsockaddr_un)[ulen & SCRATCH_SIZE_MAX] = 0;

if(is_inbound)
us_name = ((struct sockaddr_un *)sock_address)->sun_path;
else
us_name = usrsockaddr_un->sun_path;
}

int res = unix_socket_path(
&data->buf[(data->state->tail_ctx.curoff + 1 + 8 + 8) & SCRATCH_SIZE_HALF],
us_name,
UNIX_PATH_MAX);

size += res;

break;
Expand Down
55 changes: 24 additions & 31 deletions driver/ppm_events.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,10 @@ inline int sock_getname(struct socket *sock, struct sockaddr *sock_address, int
if(!u_addr) {
sunaddr->sun_family = AF_UNIX;
sunaddr->sun_path[0] = 0;
// The first byte to 0 can be confused with an `abstract socket address` for this reason
// we put also the second byte to 0 to comunicate to the caller that the address is not
// valid.
sunaddr->sun_path[1] = 0;
} else {
unsigned int len = u_addr->len;
if(unlikely(len > sizeof(struct sockaddr_storage))) {
Expand Down Expand Up @@ -858,15 +862,16 @@ static struct socket *ppm_sockfd_lookup_light(int fd, int *err, int *fput_needed

static void unix_socket_path(char *dest, const char *path, size_t size) {
if(path[0] == '\0') {
/*
* Extract from: https://man7.org/linux/man-pages/man7/unix.7.html
* an abstract socket address is distinguished (from a
/* Please note exceptions in the `sun_path`:
* Taken from: https://man7.org/linux/man-pages/man7/unix.7.html
*
* An `abstract socket address` is distinguished (from a
* pathname socket) by the fact that sun_path[0] is a null byte
* ('\0'). The socket's address in this namespace is given by
* the additional bytes in sun_path that are covered by the
* specified length of the address structure.
* ('\0').
*
* So in this case, we need to skip the initial `\0`.
*/
snprintf(dest, size, "@%s", path + 1);
snprintf(dest, size - 1, "%s", path + 1);
} else {
snprintf(dest,
size,
Expand Down Expand Up @@ -999,7 +1004,7 @@ uint16_t fd_to_socktuple(int fd,
struct socket *sock;
char *dest;
struct unix_sock *us;
char *us_name;
char *us_name = NULL;
struct sock *speer;
struct sockaddr_un *usrsockaddr_un;

Expand Down Expand Up @@ -1164,29 +1169,20 @@ uint16_t fd_to_socktuple(int fd,
if(is_inbound) {
*(uint64_t *)(targetbuf + 1) = (uint64_t)(unsigned long)us;
*(uint64_t *)(targetbuf + 1 + 8) = (uint64_t)(unsigned long)speer;
us_name = ((struct sockaddr_un *)&sock_address)->sun_path;
} else {
*(uint64_t *)(targetbuf + 1) = (uint64_t)(unsigned long)speer;
*(uint64_t *)(targetbuf + 1 + 8) = (uint64_t)(unsigned long)us;
sock_getname(sock, (struct sockaddr *)&peer_address, 1);
us_name = ((struct sockaddr_un *)&peer_address)->sun_path;
}

/*
* Pack the data into the target buffer
*/
size = 1 + 8 + 8;

if(!use_userdata) {
if(is_inbound) {
us_name = ((struct sockaddr_un *)&sock_address)->sun_path;
} else {
err = sock_getname(sock, (struct sockaddr *)&peer_address, 1);
ASSERT(err == 0);

us_name = ((struct sockaddr_un *)&peer_address)->sun_path;
}
} else {
/*
* Map the user-provided address to a sockaddr_in
*/
// `us_name` should contain the socket path extracted from the kernel if we cannot retrieve
// it we can fallback to the user-provided address
// Note that we check the second byte of `us_name`, see `sock_getname` for more details.
// Some times `usrsockaddr` is provided as a NULL pointer, checking `use_userdata` should be
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why e2e tests were failing we were dereferencing usrsockaddr without checking it was != NULL

// enough but just to be sure we check also `usrsockaddr != NULL`
if(us_name && us_name[1] == '\0' && use_userdata && usrsockaddr != NULL) {
usrsockaddr_un = (struct sockaddr_un *)usrsockaddr;

/*
Expand All @@ -1203,12 +1199,9 @@ uint16_t fd_to_socktuple(int fd,
else
us_name = usrsockaddr_un->sun_path;
}

ASSERT(us_name);

dest = targetbuf + 1 + 8 + 8;
size = 1 + 8 + 8;
dest = targetbuf + size;
unix_socket_path(dest, us_name, UNIX_PATH_MAX);

size += strlen(dest) + 1;
break;
default:
Expand Down
10 changes: 10 additions & 0 deletions test/drivers/test_suites/syscall_exit_suite/connect_x.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "../../event_class/event_class.h"
#include <libscap/strl.h>

#if defined(__NR_connect) && defined(__NR_socket) && defined(__NR_bind) && defined(__NR_close)

Expand Down Expand Up @@ -187,6 +188,14 @@ TEST(SyscallExit, connectX_UNIX) {
NOT_EQUAL,
-1);

// Create a symlink
const char* server_symlink = "/tmp/xyzxe-server-symlink";
int ret = symlink(UNIX_SERVER, server_symlink);
if(ret == -1) {
FAIL() << "Failed to create symlink";
}
strlcpy(server_addr.sun_path, server_symlink, MAX_SUN_PATH);

assert_syscall_state(
SYSCALL_SUCCESS,
"connect (client)",
Expand All @@ -198,6 +207,7 @@ TEST(SyscallExit, connectX_UNIX) {
syscall(__NR_close, client_socket_fd);
syscall(__NR_close, server_socket_fd);
syscall(__NR_unlinkat, 0, UNIX_CLIENT, 0);
syscall(__NR_unlinkat, 0, server_symlink, 0);
syscall(__NR_unlinkat, 0, UNIX_SERVER, 0);

/*=============================== TRIGGER SYSCALL ===========================*/
Expand Down
Loading