From a05f03225ed5c27436d1c8689957687937a493bc Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Fri, 10 Mar 2023 09:12:05 +0100 Subject: [PATCH 1/5] boot: Detect nested assertions --- src/boot/efi/log.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/boot/efi/log.c b/src/boot/efi/log.c index 9cbbb3a933..b805f5d084 100644 --- a/src/boot/efi/log.c +++ b/src/boot/efi/log.c @@ -10,8 +10,23 @@ _noreturn_ static void freeze(void) { BS->Stall(60 * 1000 * 1000); } +_noreturn_ static void panic(const char16_t *message) { + if (ST->ConOut->Mode->CursorColumn > 0) + ST->ConOut->OutputString(ST->ConOut, (char16_t *) u"\r\n"); + ST->ConOut->SetAttribute(ST->ConOut, EFI_TEXT_ATTR(EFI_LIGHTRED, EFI_BLACK)); + ST->ConOut->OutputString(ST->ConOut, (char16_t *) message); + freeze(); +} + void efi_assert(const char *expr, const char *file, unsigned line, const char *function) { - log_error("systemd-boot assertion '%s' failed at %s:%u@%s. Halting.", expr, file, line, function); + static bool asserting = false; + + /* Let's be paranoid. */ + if (asserting) + panic(u"systemd-boot: Nested assertion failure, halting."); + + asserting = true; + log_error("systemd-boot: Assertion '%s' failed at %s:%u@%s, halting.", expr, file, line, function); freeze(); } @@ -48,12 +63,10 @@ void log_wait(void) { /* These override the (weak) div0 handlers from libgcc as they would otherwise call raise() instead. */ _used_ _noreturn_ int __aeabi_idiv0(int return_value) { - log_error("Division by zero."); - freeze(); + panic(u"systemd-boot: Division by zero, halting."); } _used_ _noreturn_ long long __aeabi_ldiv0(long long return_value) { - log_error("Division by zero."); - freeze(); + panic(u"systemd-boot: Division by zero, halting."); } #endif From 48e1b2c250ec2fcd0b528d4062609c576afe1780 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Fri, 10 Mar 2023 09:01:29 +0100 Subject: [PATCH 2/5] boot: Add support for -fstack-protector --- meson.build | 15 +++++++++++---- src/boot/efi/log.c | 22 ++++++++++++++++++++++ src/boot/efi/log.h | 16 ++++++++++++++++ src/boot/efi/meson.build | 3 +-- src/boot/efi/util.h | 1 + 5 files changed, 51 insertions(+), 6 deletions(-) diff --git a/meson.build b/meson.build index b1521e5937..b575b04dc6 100644 --- a/meson.build +++ b/meson.build @@ -397,7 +397,14 @@ possible_common_cc_flags = [ '-Wno-error=#warnings', # clang '-Wno-string-plus-int', # clang + '-fstack-protector', + '-fstack-protector-strong', '-fstrict-flex-arrays', + '--param=ssp-buffer-size=4', +] + +possible_common_link_flags = [ + '-fstack-protector', ] c_args = get_option('c_args') @@ -432,7 +439,6 @@ possible_link_flags = [ '-Wl,--fatal-warnings', '-Wl,-z,now', '-Wl,-z,relro', - '-fstack-protector', ] if get_option('b_sanitize') == 'none' @@ -459,11 +465,8 @@ possible_cc_flags = [ '-fdiagnostics-show-option', '-fno-common', '-fno-strict-aliasing', - '-fstack-protector', - '-fstack-protector-strong', '-fstrict-flex-arrays=1', '-fvisibility=hidden', - '--param=ssp-buffer-size=4', ] if get_option('buildtype') != 'debug' @@ -486,6 +489,10 @@ add_project_arguments( ), language : 'c') +add_project_link_arguments( + cc.get_supported_link_arguments(possible_common_link_flags), + language : 'c') + userspace_c_args += cc.get_supported_arguments(possible_cc_flags) userspace_c_ld_args += cc.get_supported_link_arguments(possible_link_flags) diff --git a/src/boot/efi/log.c b/src/boot/efi/log.c index b805f5d084..6ba8d2d58e 100644 --- a/src/boot/efi/log.c +++ b/src/boot/efi/log.c @@ -1,6 +1,7 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include "log.h" +#include "proto/rng.h" #include "proto/simple-text-io.h" static unsigned log_count = 0; @@ -59,6 +60,27 @@ void log_wait(void) { log_count = 0; } +_used_ intptr_t __stack_chk_guard = (intptr_t) 0x70f6967de78acae3; + +/* We can only set a random stack canary if this function attribute is available, + * otherwise this may create a stack check fail. */ +#if STACK_PROTECTOR_RANDOM +void __stack_chk_guard_init(void) { + EFI_RNG_PROTOCOL *rng; + if (BS->LocateProtocol(MAKE_GUID_PTR(EFI_RNG_PROTOCOL), NULL, (void **) &rng) == EFI_SUCCESS) + (void) rng->GetRNG(rng, NULL, sizeof(__stack_chk_guard), (void *) &__stack_chk_guard); +} +#endif + +_used_ _noreturn_ void __stack_chk_fail(void); +_used_ _noreturn_ void __stack_chk_fail_local(void); +void __stack_chk_fail(void) { + panic(u"systemd-boot: Stack check failed, halting."); +} +void __stack_chk_fail_local(void) { + __stack_chk_fail(); +} + #if defined(__ARM_EABI__) /* These override the (weak) div0 handlers from libgcc as they would otherwise call raise() instead. */ diff --git a/src/boot/efi/log.h b/src/boot/efi/log.h index 9bdcfad923..7b2735d028 100644 --- a/src/boot/efi/log.h +++ b/src/boot/efi/log.h @@ -3,6 +3,22 @@ #include "efi-string.h" +#if defined __has_attribute +# if __has_attribute(no_stack_protector) +# define HAVE_NO_STACK_PROTECTOR_ATTRIBUTE +# endif +#endif + +#if defined(HAVE_NO_STACK_PROTECTOR_ATTRIBUTE) && \ + (defined(__SSP__) || defined(__SSP_ALL__) || \ + defined(__SSP_STRONG__) || defined(__SSP_EXPLICIT__)) +# define STACK_PROTECTOR_RANDOM 1 +__attribute__((no_stack_protector, noinline)) void __stack_chk_guard_init(void); +#else +# define STACK_PROTECTOR_RANDOM 0 +# define __stack_chk_guard_init() +#endif + void log_wait(void); _gnu_printf_(2, 3) EFI_STATUS log_internal(EFI_STATUS status, const char *format, ...); #define log_error_status(status, ...) log_internal(status, __VA_ARGS__) diff --git a/src/boot/efi/meson.build b/src/boot/efi/meson.build index 7e497f7866..bfc3f9c279 100644 --- a/src/boot/efi/meson.build +++ b/src/boot/efi/meson.build @@ -139,6 +139,7 @@ efi_c_args += cc.get_supported_arguments( '-fwide-exec-charset=UCS2', # gcc docs says this is required for ms_abi to work correctly. '-maccumulate-outgoing-args', + '-mstack-protector-guard=global', ) # Debug information has little value in release builds as no normal human being knows @@ -180,8 +181,6 @@ efi_disabled_c_args = cc.get_supported_arguments( '-fno-exceptions', '-fno-trapv', '-fno-sanitize=all', - '-fno-stack-clash-protection', - '-fno-stack-protector', '-fno-unwind-tables', ) efi_c_args += efi_disabled_c_args diff --git a/src/boot/efi/util.h b/src/boot/efi/util.h index 5e1085c788..5b4f47a1ae 100644 --- a/src/boot/efi/util.h +++ b/src/boot/efi/util.h @@ -176,6 +176,7 @@ void hexdump(const char16_t *prefix, const void *data, size_t size); ST = system_table; \ BS = system_table->BootServices; \ RT = system_table->RuntimeServices; \ + __stack_chk_guard_init(); \ notify_debugger((identity), (wait_for_debugger)); \ EFI_STATUS err = func(image); \ log_wait(); \ From f64f82aa8d5181bb024767292aa8bcabdc7c019c Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Fri, 10 Mar 2023 09:21:08 +0100 Subject: [PATCH 3/5] boot: Add support for -ftrapv --- src/boot/efi/log.c | 6 ++++++ src/boot/efi/meson.build | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/boot/efi/log.c b/src/boot/efi/log.c index 6ba8d2d58e..879ed766e3 100644 --- a/src/boot/efi/log.c +++ b/src/boot/efi/log.c @@ -81,6 +81,12 @@ void __stack_chk_fail_local(void) { __stack_chk_fail(); } +/* Called by libgcc for some fatal errors like integer overflow with -ftrapv. */ +_used_ _noreturn_ void abort(void); +void abort(void) { + panic(u"systemd-boot: Unknown error, halting."); +} + #if defined(__ARM_EABI__) /* These override the (weak) div0 handlers from libgcc as they would otherwise call raise() instead. */ diff --git a/src/boot/efi/meson.build b/src/boot/efi/meson.build index bfc3f9c279..58bebe446e 100644 --- a/src/boot/efi/meson.build +++ b/src/boot/efi/meson.build @@ -179,7 +179,6 @@ efi_disabled_c_args = cc.get_supported_arguments( '-fcf-protection=none', '-fno-asynchronous-unwind-tables', '-fno-exceptions', - '-fno-trapv', '-fno-sanitize=all', '-fno-unwind-tables', ) From 0b482b37f9289e25bc7375609b2fc46f88affad9 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Fri, 10 Mar 2023 09:57:50 +0100 Subject: [PATCH 4/5] meson: Share more C flags --- meson.build | 8 ++++---- src/boot/efi/efi-string.c | 12 ++++++++---- src/boot/efi/log.c | 6 ++++-- src/boot/efi/util.h | 1 + src/boot/efi/vmm.c | 1 + 5 files changed, 18 insertions(+), 10 deletions(-) diff --git a/meson.build b/meson.build index b575b04dc6..6770deed4f 100644 --- a/meson.build +++ b/meson.build @@ -366,6 +366,8 @@ possible_common_cc_flags = [ '-Werror=implicit-int', '-Werror=incompatible-pointer-types', '-Werror=int-conversion', + '-Werror=missing-declarations', + '-Werror=missing-prototypes', '-Werror=overflow', '-Werror=override-init', '-Werror=return-type', @@ -397,6 +399,8 @@ possible_common_cc_flags = [ '-Wno-error=#warnings', # clang '-Wno-string-plus-int', # clang + '-fdiagnostics-show-option', + '-fno-common', '-fstack-protector', '-fstack-protector-strong', '-fstrict-flex-arrays', @@ -460,10 +464,6 @@ if get_option('mode') == 'release' endif possible_cc_flags = [ - '-Werror=missing-declarations', - '-Werror=missing-prototypes', - '-fdiagnostics-show-option', - '-fno-common', '-fno-strict-aliasing', '-fstrict-flex-arrays=1', '-fvisibility=hidden', diff --git a/src/boot/efi/efi-string.c b/src/boot/efi/efi-string.c index a94e2e4c17..d199410881 100644 --- a/src/boot/efi/efi-string.c +++ b/src/boot/efi/efi-string.c @@ -882,6 +882,10 @@ char16_t *xvasprintf_status(EFI_STATUS status, const char *format, va_list ap) { # undef memcmp # undef memcpy # undef memset +_used_ void *memchr(const void *p, int c, size_t n); +_used_ int memcmp(const void *p1, const void *p2, size_t n); +_used_ void *memcpy(void * restrict dest, const void * restrict src, size_t n); +_used_ void *memset(void *p, int c, size_t n); #else /* And for userspace unit testing we need to give them an efi_ prefix. */ # define memchr efi_memchr @@ -890,7 +894,7 @@ char16_t *xvasprintf_status(EFI_STATUS status, const char *format, va_list ap) { # define memset efi_memset #endif -_used_ void *memchr(const void *p, int c, size_t n) { +void *memchr(const void *p, int c, size_t n) { if (!p || n == 0) return NULL; @@ -902,7 +906,7 @@ _used_ void *memchr(const void *p, int c, size_t n) { return NULL; } -_used_ int memcmp(const void *p1, const void *p2, size_t n) { +int memcmp(const void *p1, const void *p2, size_t n) { const uint8_t *up1 = p1, *up2 = p2; int r; @@ -922,7 +926,7 @@ _used_ int memcmp(const void *p1, const void *p2, size_t n) { return 0; } -_used_ void *memcpy(void * restrict dest, const void * restrict src, size_t n) { +void *memcpy(void * restrict dest, const void * restrict src, size_t n) { if (!dest || !src || n == 0) return dest; @@ -949,7 +953,7 @@ _used_ void *memcpy(void * restrict dest, const void * restrict src, size_t n) { return dest; } -_used_ void *memset(void *p, int c, size_t n) { +void *memset(void *p, int c, size_t n) { if (!p || n == 0) return p; diff --git a/src/boot/efi/log.c b/src/boot/efi/log.c index 879ed766e3..82307516dc 100644 --- a/src/boot/efi/log.c +++ b/src/boot/efi/log.c @@ -89,12 +89,14 @@ void abort(void) { #if defined(__ARM_EABI__) /* These override the (weak) div0 handlers from libgcc as they would otherwise call raise() instead. */ +_used_ _noreturn_ int __aeabi_idiv0(int return_value); +_used_ _noreturn_ long long __aeabi_ldiv0(long long return_value); -_used_ _noreturn_ int __aeabi_idiv0(int return_value) { +int __aeabi_idiv0(int return_value) { panic(u"systemd-boot: Division by zero, halting."); } -_used_ _noreturn_ long long __aeabi_ldiv0(long long return_value) { +long long __aeabi_ldiv0(long long return_value) { panic(u"systemd-boot: Division by zero, halting."); } #endif diff --git a/src/boot/efi/util.h b/src/boot/efi/util.h index 5b4f47a1ae..c321062996 100644 --- a/src/boot/efi/util.h +++ b/src/boot/efi/util.h @@ -172,6 +172,7 @@ void hexdump(const char16_t *prefix, const void *data, size_t size); EFI_SYSTEM_TABLE *ST; \ EFI_BOOT_SERVICES *BS; \ EFI_RUNTIME_SERVICES *RT; \ + EFIAPI EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *system_table); \ EFIAPI EFI_STATUS efi_main(EFI_HANDLE image, EFI_SYSTEM_TABLE *system_table) { \ ST = system_table; \ BS = system_table->BootServices; \ diff --git a/src/boot/efi/vmm.c b/src/boot/efi/vmm.c index 60e8a97c43..951b4e3766 100644 --- a/src/boot/efi/vmm.c +++ b/src/boot/efi/vmm.c @@ -10,6 +10,7 @@ #include "proto/device-path.h" #include "string-util-fundamental.h" #include "util.h" +#include "vmm.h" #define QEMU_KERNEL_LOADER_FS_MEDIA_GUID \ { 0x1428f772, 0xb64a, 0x441e, { 0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7 } } From 1e7ff4ba8855996c8e077e0546acf9491c97663f Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Sun, 12 Mar 2023 16:51:48 +0100 Subject: [PATCH 5/5] boot: Add undefined sanitizer support Sadly, no stack traces, but this is better than nothing. --- src/boot/efi/log.c | 2 +- src/boot/efi/log.h | 1 + src/boot/efi/meson.build | 18 ++++++++++++---- src/boot/efi/ubsan.c | 46 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 src/boot/efi/ubsan.c diff --git a/src/boot/efi/log.c b/src/boot/efi/log.c index 82307516dc..6d0edec2cf 100644 --- a/src/boot/efi/log.c +++ b/src/boot/efi/log.c @@ -6,7 +6,7 @@ static unsigned log_count = 0; -_noreturn_ static void freeze(void) { +void freeze(void) { for (;;) BS->Stall(60 * 1000 * 1000); } diff --git a/src/boot/efi/log.h b/src/boot/efi/log.h index 7b2735d028..973e13c260 100644 --- a/src/boot/efi/log.h +++ b/src/boot/efi/log.h @@ -19,6 +19,7 @@ __attribute__((no_stack_protector, noinline)) void __stack_chk_guard_init(void); # define __stack_chk_guard_init() #endif +_noreturn_ void freeze(void); void log_wait(void); _gnu_printf_(2, 3) EFI_STATUS log_internal(EFI_STATUS status, const char *format, ...); #define log_error_status(status, ...) log_internal(status, __VA_ARGS__) diff --git a/src/boot/efi/meson.build b/src/boot/efi/meson.build index 58bebe446e..e6e7eed3bc 100644 --- a/src/boot/efi/meson.build +++ b/src/boot/efi/meson.build @@ -179,17 +179,23 @@ efi_disabled_c_args = cc.get_supported_arguments( '-fcf-protection=none', '-fno-asynchronous-unwind-tables', '-fno-exceptions', - '-fno-sanitize=all', '-fno-unwind-tables', ) -efi_c_args += efi_disabled_c_args -efi_c_ld_args += efi_disabled_c_args efi_override_options = [ 'b_coverage=false', 'b_pgo=off', - 'b_sanitize=none', ] +if get_option('b_sanitize') == 'undefined' + efi_disabled_c_args += cc.get_supported_arguments('-fno-sanitize-link-runtime') +else + efi_disabled_c_args += cc.get_supported_arguments('-fno-sanitize=all') + efi_override_options += 'b_sanitize=none' +endif + +efi_c_args += efi_disabled_c_args +efi_c_ld_args += efi_disabled_c_args + if cc.get_id() == 'clang' # clang is too picky sometimes. efi_c_args += '-Wno-unused-command-line-argument' @@ -252,6 +258,10 @@ stub_sources = files( 'stub.c', ) +if get_option('b_sanitize') == 'undefined' + libefi_sources += files('ubsan.c') +endif + if host_machine.cpu_family() in ['x86', 'x86_64'] stub_sources += files('linux_x86.c') endif diff --git a/src/boot/efi/ubsan.c b/src/boot/efi/ubsan.c new file mode 100644 index 0000000000..951204683e --- /dev/null +++ b/src/boot/efi/ubsan.c @@ -0,0 +1,46 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "log.h" + +typedef struct { + const char *filename; + uint32_t line; + uint32_t column; +} SourceLocation; + +/* Note that all ubsan handlers have a pointer to a type-specific struct passed as first argument. + * Since we do not inspect the extra data in it we can just treat it as a SourceLocation struct + * directly to keep things simple. */ + +#define HANDLER(name, ...) \ + _used_ _noreturn_ void __ubsan_handle_##name(__VA_ARGS__); \ + void __ubsan_handle_##name(__VA_ARGS__) { \ + log_error("systemd-boot: %s in %s@%u:%u", \ + __func__, \ + location->filename, \ + location->line, \ + location->column); \ + freeze(); \ + } + +#define UNARY_HANDLER(name) HANDLER(name, SourceLocation *location, uintptr_t v) +#define BINARY_HANDLER(name) HANDLER(name, SourceLocation *location, uintptr_t v1, uintptr_t v2) + +UNARY_HANDLER(load_invalid_value); +UNARY_HANDLER(negate_overflow); +UNARY_HANDLER(out_of_bounds); +UNARY_HANDLER(type_mismatch_v1); +UNARY_HANDLER(vla_bound_not_positive); + +BINARY_HANDLER(add_overflow); +BINARY_HANDLER(divrem_overflow); +BINARY_HANDLER(implicit_conversion); +BINARY_HANDLER(mul_overflow); +BINARY_HANDLER(pointer_overflow); +BINARY_HANDLER(shift_out_of_bounds); +BINARY_HANDLER(sub_overflow); + +HANDLER(builtin_unreachable, SourceLocation *location); +HANDLER(invalid_builtin, SourceLocation *location); +HANDLER(nonnull_arg, SourceLocation *location); +HANDLER(nonnull_return_v1, SourceLocation *attr_location, SourceLocation *location);