From f05ae1cb639769d2a85526776c4f76551c79f19d Mon Sep 17 00:00:00 2001 From: Dmitrii Kuvaiskii Date: Thu, 27 Jun 2024 05:31:04 -0700 Subject: [PATCH] [common] Update UBSan to be compatible with Clang 18 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 scaffolding 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 --- common/src/avl_tree.c | 10 +++++++++ common/src/ubsan.c | 36 +++++++++++++++++++++++++++++++ libos/src/fs/sys/fs.c | 2 +- libos/src/libos_syscalls.c | 8 +++++++ libos/src/net/unix.c | 17 +++++++++------ pal/src/host/linux-sgx/pal_main.c | 2 +- pal/src/pal_rtld.c | 2 +- 7 files changed, 67 insertions(+), 10 deletions(-) diff --git a/common/src/avl_tree.c b/common/src/avl_tree.c index 56f1dbbe59..fe1565de57 100644 --- a/common/src/avl_tree.c +++ b/common/src/avl_tree.c @@ -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; diff --git a/common/src/ubsan.c b/common/src/ubsan.c index df9c54ce89..acc6788bd2 100644 --- a/common/src/ubsan.c +++ b/common/src/ubsan.c @@ -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 { @@ -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) { @@ -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); @@ -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(); +} diff --git a/libos/src/fs/sys/fs.c b/libos/src/fs/sys/fs.c index b941f60234..c6400f98f2 100644 --- a/libos/src/fs/sys/fs.c +++ b/libos/src/fs/sys/fs.c @@ -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. */ diff --git a/libos/src/libos_syscalls.c b/libos/src/libos_syscalls.c index 83be137621..cef57712fe 100644 --- a/libos/src/libos_syscalls.c +++ b/libos/src/libos_syscalls.c @@ -21,6 +21,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); diff --git a/libos/src/net/unix.c b/libos/src/net/unix.c index af7a1e1bed..bb71ff132d 100644 --- a/libos/src/net/unix.c +++ b/libos/src/net/unix.c @@ -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: @@ -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) { @@ -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; @@ -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. */ diff --git a/pal/src/host/linux-sgx/pal_main.c b/pal/src/host/linux-sgx/pal_main.c index a5d32f47f7..92a4333927 100644 --- a/pal/src/host/linux-sgx/pal_main.c +++ b/pal/src/host/linux-sgx/pal_main.c @@ -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); diff --git a/pal/src/pal_rtld.c b/pal/src/pal_rtld.c index f6f9df5f5b..9cb2f20258 100644 --- a/pal/src/pal_rtld.c +++ b/pal/src/pal_rtld.c @@ -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 "