From 3b23a6c40ac42c5673c6817ddf174983497519da Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Feb 2022 15:28:19 +0100 Subject: [PATCH 1/3] fundamental: support assert_se() in EFI mode too --- src/boot/efi/assert.c | 5 +---- src/fundamental/macro-fundamental.h | 4 +++- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/boot/efi/assert.c b/src/boot/efi/assert.c index 7a25526de2..bb16d2bf93 100644 --- a/src/boot/efi/assert.c +++ b/src/boot/efi/assert.c @@ -1,9 +1,8 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ -#ifndef NDEBUG - #include #include + #include "util.h" void efi_assert(const char *expr, const char *file, unsigned line, const char *function) { @@ -11,5 +10,3 @@ void efi_assert(const char *expr, const char *file, unsigned line, const char *f for (;;) BS->Stall(60 * 1000 * 1000); } - -#endif diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h index d597c743bb..a1cbc3a5b3 100644 --- a/src/fundamental/macro-fundamental.h +++ b/src/fundamental/macro-fundamental.h @@ -53,14 +53,16 @@ #define CONCATENATE(x, y) XCONCATENATE(x, y) #ifdef SD_BOOT + void efi_assert(const char *expr, const char *file, unsigned line, const char *function) _noreturn_; + #ifdef NDEBUG #define assert(expr) #define assert_not_reached() __builtin_unreachable() #else - void efi_assert(const char *expr, const char *file, unsigned line, const char *function) _noreturn_; #define assert(expr) ({ _likely_(expr) ? VOID_0 : efi_assert(#expr, __FILE__, __LINE__, __PRETTY_FUNCTION__); }) #define assert_not_reached() efi_assert("Code should not be reached", __FILE__, __LINE__, __PRETTY_FUNCTION__) #endif + #define assert_se(expr) ({ _likely_(expr) ? VOID_0 : efi_assert(#expr, __FILE__, __LINE__, __PRETTY_FUNCTION__); }) #define memcpy(a, b, c) CopyMem((a), (b), (c)) #define free(a) FreePool(a) From 8890ec82f5f8a04c44dc527afd5ad868505e74d0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Feb 2022 14:43:04 +0100 Subject: [PATCH 2/3] macro: add ASSERT_SE_PTR() macro ASSERT_SE_PTR() is like ASSERT_PTR() but uses assert_se() instead of assert() internally. Code should use ASSERT_SE_PTR() where the check should never be optimized away, even if NDEBUG is set. Rationale: assert() is the right choice for validating assumptions about our own code, i.e. checking conditions that are "impossible" to not hold, because we ourselves hacked things up the "right" way of course. assert_se() is the right choice for tests that come with a weaker guarantee, they encode assumptions over other's API behaviour, i.e. whether something can fail there or not. When developing tools that are not oom-safe assert_se() is the right choice: we know that on Linux OOM doesn't really happen, even though theoretically the API allows it to happen. Usecase for ASSERT_SE_PTR() is mostly the fatal memory allocation logic for EFI memory allocations. So far it used regular assert() i.e. OOM failurs would be totally ignored if NDEBUG is set. We'd rather have our EFI program to print an assert message and freeze instead though. --- src/fundamental/macro-fundamental.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/fundamental/macro-fundamental.h b/src/fundamental/macro-fundamental.h index a1cbc3a5b3..15b93165dc 100644 --- a/src/fundamental/macro-fundamental.h +++ b/src/fundamental/macro-fundamental.h @@ -76,6 +76,13 @@ _expr_; \ }) +#define ASSERT_SE_PTR(expr) \ + ({ \ + typeof(expr) _expr_ = (expr); \ + assert_se(_expr_); \ + _expr_; \ + }) + #if defined(static_assert) #define assert_cc(expr) \ static_assert(expr, #expr) From 1462d2451a36aada0e5eec2a242aeb9366f6df44 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 1 Feb 2022 15:28:33 +0100 Subject: [PATCH 3/3] efi: use assert_se() instead of assert() to guard for OOM issues in EFI code --- src/boot/efi/util.h | 13 ++++++------- src/boot/efi/xbootldr.c | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/boot/efi/util.h b/src/boot/efi/util.h index 40ce98c611..3d659f7043 100644 --- a/src/boot/efi/util.h +++ b/src/boot/efi/util.h @@ -27,16 +27,15 @@ #define xnew_alloc(type, n, alloc) \ ({ \ UINTN _alloc_size; \ - if (__builtin_mul_overflow(sizeof(type), (n), &_alloc_size)) \ - assert_not_reached(); \ + assert_se(!__builtin_mul_overflow(sizeof(type), (n), &_alloc_size)); \ (type *) alloc(_alloc_size); \ }) -#define xallocate_pool(size) ASSERT_PTR(AllocatePool(size)) -#define xallocate_zero_pool(size) ASSERT_PTR(AllocateZeroPool(size)) -#define xreallocate_pool(p, old_size, new_size) ASSERT_PTR(ReallocatePool((p), (old_size), (new_size))) -#define xpool_print(fmt, ...) ((CHAR16 *) ASSERT_PTR(PoolPrint((fmt), ##__VA_ARGS__))) -#define xstrdup(str) ((CHAR16 *) ASSERT_PTR(StrDuplicate(str))) +#define xallocate_pool(size) ASSERT_SE_PTR(AllocatePool(size)) +#define xallocate_zero_pool(size) ASSERT_SE_PTR(AllocateZeroPool(size)) +#define xreallocate_pool(p, old_size, new_size) ASSERT_SE_PTR(ReallocatePool((p), (old_size), (new_size))) +#define xpool_print(fmt, ...) ((CHAR16 *) ASSERT_SE_PTR(PoolPrint((fmt), ##__VA_ARGS__))) +#define xstrdup(str) ((CHAR16 *) ASSERT_SE_PTR(StrDuplicate(str))) #define xnew(type, n) xnew_alloc(type, (n), xallocate_pool) #define xnew0(type, n) xnew_alloc(type, (n), xallocate_zero_pool) diff --git a/src/boot/efi/xbootldr.c b/src/boot/efi/xbootldr.c index 49d5707e02..6fa7e49e5d 100644 --- a/src/boot/efi/xbootldr.c +++ b/src/boot/efi/xbootldr.c @@ -226,7 +226,7 @@ static EFI_STATUS find_device(EFI_HANDLE *device, EFI_DEVICE_PATH **ret_device_p } /* Patch in the data we found */ - EFI_DEVICE_PATH *xboot_path = ASSERT_PTR(DuplicateDevicePath(partition_path)); + EFI_DEVICE_PATH *xboot_path = ASSERT_SE_PTR(DuplicateDevicePath(partition_path)); CopyMem((UINT8 *) xboot_path + ((UINT8 *) part_node - (UINT8 *) partition_path), &hd, sizeof(hd)); *ret_device_path = xboot_path; return EFI_SUCCESS;