Skip to content

Commit

Permalink
[common] Update UBSan to be compatible with Clang 18
Browse files Browse the repository at this point in the history
Newer Clang versions added more UBSan checks, in particular:
- `-fsanitize=pointer-overflow` check was extended to catch the cases
  where a non-zero offset is applied to a null pointer, or the result of
  applying the offset is a null pointer.
- `fsanitize=function`: Indirect call of a function through a function
  pointer of the wrong type.

This commit adds the necessary support for the second (new) check plus
fixes the places triggered by this check. This commit also fixes UBs
found by the extended first check.

Signed-off-by: Dmitrii Kuvaiskii <dmitrii.kuvaiskii@intel.com>
  • Loading branch information
Dmitrii Kuvaiskii committed Jul 22, 2024
1 parent afb8a35 commit 78776a5
Show file tree
Hide file tree
Showing 7 changed files with 67 additions and 10 deletions.
10 changes: 10 additions & 0 deletions common/src/avl_tree.c
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,16 @@ struct avl_tree_node* avl_tree_find(struct avl_tree* tree, struct avl_tree_node*
return avl_tree_find_fn_to(tree, node, tree->cmp);
}

#ifdef UBSAN
/*
* Function pointer cmp is of type `bool (*)(void*, struct avl_tree_node*)` but the caller
* avl_tree_lower_bound() passes cmp as `bool (*)(struct avl_tree_node*, struct avl_tree_node*)`.
* UBSan complains about "Indirect call of a function through a function pointer of the wrong type"
* because of this mismatch. However, allowing the first argument to be a pointer to any type was
* done on purpose; see corresponding comment in avl_tree.h. So, silence this particular complaint.
*/
__attribute__((no_sanitize("function")))
#endif
struct avl_tree_node* avl_tree_lower_bound_fn(struct avl_tree* tree, void* cmp_arg,
bool cmp(void*, struct avl_tree_node*)) {
struct avl_tree_node* node = tree->root;
Expand Down
36 changes: 36 additions & 0 deletions common/src/ubsan.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@

/* Type definitions (adapted from libubsan) */

/*
* NOTE: TypeDescriptor contains information about the type of the variable/function, in particular,
* `char* TypeName`. Unfortunately, TypeDescriptor is implemented as C++ class in GCC/Clang so we
* can't easily represent it in C. Thus, we declare it as opaque struct and can't use TypeName.
*/
struct type_descriptor;

struct source_location {
Expand All @@ -41,6 +46,11 @@ struct type_mismatch_data {
uint8_t type_check_kind;
};

struct function_type_mismatch_data {
struct source_location loc;
const struct type_descriptor* type;
};

typedef uintptr_t value_handle;

static void ubsan_log_location(struct source_location* loc) {
Expand Down Expand Up @@ -131,6 +141,7 @@ UBSAN_SIMPLE_HANDLER_1(invalid_builtin,

/* More complex handlers, displaying additional information. */

/* type mismatch handlers */
void __ubsan_handle_type_mismatch_v1(struct type_mismatch_data* data, value_handle pointer);
void __ubsan_handle_type_mismatch_v1_abort(struct type_mismatch_data* data, value_handle pointer);

Expand All @@ -151,3 +162,28 @@ void __ubsan_handle_type_mismatch_v1_abort(struct type_mismatch_data* data, valu
__ubsan_handle_type_mismatch_v1(data, pointer);
abort();
}

/* function type mismatch handlers */
void __ubsan_handle_function_type_mismatch(struct function_type_mismatch_data* data,
value_handle function_pointer);
void __ubsan_handle_function_type_mismatch_abort(struct function_type_mismatch_data* data,
value_handle function_pointer);

void __ubsan_handle_function_type_mismatch(struct function_type_mismatch_data* data,
value_handle function_pointer) {
/*
* NOTE: UBSan in GCC/Clang prints "call to function {getSymbolizedLocation(function_pointer)}
* through pointer to incorrect function type {data->type->TypeName}". Unfortunately we can't
* learn the name of the func based on the address, and we can't use TypeName (see comment at
* the top of this file).
*/
log_error("ubsan: call to function at 0x%lx through pointer to incorrect function type",
function_pointer);
ubsan_log_location(&data->loc);
}

void __ubsan_handle_function_type_mismatch_abort(struct function_type_mismatch_data* data,
value_handle function_pointer) {
__ubsan_handle_function_type_mismatch(data, function_pointer);
abort();
}
2 changes: 1 addition & 1 deletion libos/src/fs/sys/fs.c
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ int sys_print_as_bitmask(char* buf, size_t buf_size, size_t count,
uint32_t word = 0;
while (1) {
if (is_present(pos, callback_arg))
word |= 1 << pos % 32;
word |= 1U << pos % 32;
if (pos % 32 == 0) {
if (count <= 32) {
/* Linux sysfs quirk: small bitmasks are printed without leading zeroes. */
Expand Down
8 changes: 8 additions & 0 deletions libos/src/libos_syscalls.c
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ typedef arch_syscall_arg_t (*six_args_syscall_t)(arch_syscall_arg_t, arch_syscal
* `context` is expected to be placed at the bottom of Gramine-internal stack.
* If you change this function please also look at `libos_syscall_rt_sigsuspend`!
*/
#ifdef UBSAN
/*
* Variable syscall_func is of type six_args_syscall_t but points to item inside libos_syscall_table
* array, which has a type libos_syscall_t, thus UBSan complains about "Indirect call of a function
* through a function pointer of the wrong type". Silence this particular complaint.
*/
__attribute__((no_sanitize("function")))
#endif
noreturn void libos_emulate_syscall(PAL_CONTEXT* context) {
LIBOS_TCB_SET(context.regs, context);

Expand Down
17 changes: 10 additions & 7 deletions libos/src/net/unix.c
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,7 @@ static int getsockopt(struct libos_handle* handle, int level, int optname, void*
}

static int maybe_force_nonblocking_wrapper(bool force_nonblocking, struct libos_handle* handle,
PAL_HANDLE pal_handle,
int (*func)(PAL_HANDLE, uint64_t, size_t*, void*),
PAL_HANDLE pal_handle, bool is_pal_stream_read,
void* buf, size_t* size) {
/*
* There are 3 kinds of operations that can race here:
Expand Down Expand Up @@ -367,7 +366,11 @@ static int maybe_force_nonblocking_wrapper(bool force_nonblocking, struct libos_
}

again:
ret = func(pal_handle, /*offset=*/0, size, buf);
if (is_pal_stream_read) {
ret = PalStreamRead(pal_handle, /*offset=*/0, size, buf);
} else {
ret = PalStreamWrite(pal_handle, /*offset=*/0, size, buf);
}
if (ret < 0) {
ret = (ret == -PAL_ERROR_TOOLONG) ? -EMSGSIZE : pal_to_unix_errno(ret);
if (ret == -EAGAIN && !force_nonblocking) {
Expand Down Expand Up @@ -475,8 +478,8 @@ static int send(struct libos_handle* handle, struct iovec* iov, size_t iov_len,
/* `size` is already correct. */
}

int ret = maybe_force_nonblocking_wrapper(force_nonblocking, handle, pal_handle, PalStreamWrite,
buf, &size);
int ret = maybe_force_nonblocking_wrapper(force_nonblocking, handle, pal_handle,
/*is_pal_stream_read=*/false, buf, &size);
free(backing_buf);
if (ret < 0) {
return ret;
Expand Down Expand Up @@ -521,8 +524,8 @@ static int recv(struct libos_handle* handle, struct iovec* iov, size_t iov_len,
/* `size` is already correct. */
}

int ret = maybe_force_nonblocking_wrapper(force_nonblocking, handle, pal_handle, PalStreamRead,
buf, &size);
int ret = maybe_force_nonblocking_wrapper(force_nonblocking, handle, pal_handle,
/*is_pal_stream_read=*/true, buf, &size);
if (ret == 0) {
if (backing_buf) {
/* Need to copy back to user buffers. */
Expand Down
2 changes: 1 addition & 1 deletion pal/src/host/linux-sgx/pal_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -903,7 +903,7 @@ noreturn void pal_linux_main(void* uptr_libpal_uri, size_t libpal_uri_len, void*
}

init_handle_hdr(first_thread, PAL_TYPE_THREAD);
first_thread->thread.tcs = (void*)(g_enclave_base + GET_ENCLAVE_TCB(tcs_offset));
first_thread->thread.tcs = (void*)((uintptr_t)g_enclave_base + GET_ENCLAVE_TCB(tcs_offset));
g_pal_public_state.first_thread = first_thread;
SET_ENCLAVE_TCB(thread, &first_thread->thread);

Expand Down
2 changes: 1 addition & 1 deletion pal/src/pal_rtld.c
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ static int perform_relocations(struct link_map* map) {
}

/* perform PLT relocs: supported binaries may have only R_X86_64_JUMP_SLOT relas */
elf_rela_t* plt_relas_addr_end = (void*)plt_relas_addr + plt_relas_size;
elf_rela_t* plt_relas_addr_end = (elf_rela_t*)((uintptr_t)plt_relas_addr + plt_relas_size);
for (elf_rela_t* plt_rela = plt_relas_addr; plt_rela < plt_relas_addr_end; plt_rela++) {
if (ELF_R_TYPE(plt_rela->r_info) != R_X86_64_JUMP_SLOT) {
log_error("Unrecognized relocation type; PAL loader currently supports only "
Expand Down

0 comments on commit 78776a5

Please sign in to comment.