From 77fcf28cb88b302453b4c991a6571cb37f10634d Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Tue, 28 Dec 2021 13:10:39 +0100 Subject: [PATCH 1/3] boot: Build BCD parser only on arches supported by Windows --- src/boot/efi/boot.c | 2 ++ src/boot/efi/meson.build | 21 ++++++++++++--------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 17d14a5dac..0360d2a0bf 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1941,6 +1941,7 @@ static void config_entry_add_osx(Config *config) { } static void config_entry_add_windows(Config *config, EFI_HANDLE *device, EFI_FILE *root_dir) { +#if defined(__i386__) || defined(__x86_64__) || defined(__arm__) || defined(__aarch64__) _cleanup_freepool_ CHAR8 *bcd = NULL; CHAR16 *title = NULL; EFI_STATUS err; @@ -1961,6 +1962,7 @@ static void config_entry_add_windows(Config *config, EFI_HANDLE *device, EFI_FIL config_entry_add_loader_auto(config, device, root_dir, NULL, L"auto-windows", 'w', title ?: L"Windows Boot Manager", L"\\EFI\\Microsoft\\Boot\\bootmgfw.efi"); +#endif } static void config_entry_add_linux( diff --git a/src/boot/efi/meson.build b/src/boot/efi/meson.build index a6a0fa2595..03e603a376 100644 --- a/src/boot/efi/meson.build +++ b/src/boot/efi/meson.build @@ -331,7 +331,6 @@ common_sources = [ 'util.c'] systemd_boot_sources = [ - 'bcd.c', 'boot.c', 'console.c', 'drivers.c', @@ -351,6 +350,18 @@ else stub_sources += 'linux.c' endif +# BCD parser only makes sense on arches that Windows supports. +if efi_arch[1] in ['ia32', 'x86_64', 'arm', 'aarch64'] + systemd_boot_sources += 'bcd.c' + tests += [ + [['src/boot/efi/test-bcd.c'], + [], + [libzstd], + [], + 'HAVE_ZSTD'], + ] +endif + systemd_boot_objects = [] stub_objects = [] foreach file : fundamental_source_paths + common_sources + systemd_boot_sources + stub_sources @@ -407,14 +418,6 @@ endforeach ############################################################ -tests += [ - [['src/boot/efi/test-bcd.c'], - [], - [libzstd], - [], - 'HAVE_ZSTD'], -] - test_efi_disk_img = custom_target( 'test-efi-disk.img', input : [efi_stubs[0][0], efi_stubs[1][1]], From 1cadb35fd68f0255e50627dffd25c83e7e2081e5 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Tue, 28 Dec 2021 16:07:09 +0100 Subject: [PATCH 2/3] boot: Reject unaligned data The data seems to be properly aligned in real BCD stores, so it should be fine to just reject bad ones. Fixes: #21917 --- src/boot/efi/bcd.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/boot/efi/bcd.c b/src/boot/efi/bcd.c index 44c544f8f7..85569deb09 100644 --- a/src/boot/efi/bcd.c +++ b/src/boot/efi/bcd.c @@ -176,7 +176,8 @@ static const KeyValue *get_key_value(const UINT8 *bcd, UINT32 bcd_len, const Key if (key->n_key_values == 0) return NULL; - if ((UINT64) key->key_values_offset + sizeof(UINT32[key->n_key_values]) >= bcd_len) + if ((UINT64) key->key_values_offset + sizeof(UINT32[key->n_key_values]) >= bcd_len || + (UINTN)(bcd + key->key_values_offset) % sizeof(UINT32) != 0) return NULL; const UINT32 *key_value_list = (const UINT32 *) (bcd + key->key_values_offset); @@ -266,7 +267,8 @@ TEST_STATIC CHAR16 *get_bcd_title(UINT8 *bcd, UINTN bcd_len) { CHAR8 order_guid[sizeof("{00000000-0000-0000-0000-000000000000}\0")]; if (displayorder_value->data_type != REG_MULTI_SZ || - displayorder_value->data_size != sizeof(CHAR16) * sizeof(order_guid)) + displayorder_value->data_size != sizeof(CHAR16[sizeof(order_guid)]) || + (UINTN)(bcd + displayorder_value->data_offset) % sizeof(CHAR16) != 0) /* BCD is multi-boot. */ return NULL; @@ -312,7 +314,8 @@ TEST_STATIC CHAR16 *get_bcd_title(UINT8 *bcd, UINTN bcd_len) { if (description_value->data_type != REG_SZ || description_value->data_size < sizeof(CHAR16) || - description_value->data_size % sizeof(CHAR16) != 0) + description_value->data_size % sizeof(CHAR16) != 0 || + (UINTN)(bcd + description_value->data_offset) % sizeof(CHAR16)) return NULL; /* The data should already be NUL-terminated. */ From aa1d0f25873f737fb9306a12f9283872012f2d9a Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 29 Dec 2021 14:35:07 +0100 Subject: [PATCH 3/3] boot: Introduce helper macros for offset checking This fixes a subtle sizeof overflow on 32bit machines. --- src/boot/efi/bcd.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/src/boot/efi/bcd.c b/src/boot/efi/bcd.c index 85569deb09..3eaabdd538 100644 --- a/src/boot/efi/bcd.c +++ b/src/boot/efi/bcd.c @@ -112,20 +112,30 @@ assert_cc(offsetof(KeyValue, data_offset) == 8); assert_cc(offsetof(KeyValue, data_type) == 12); assert_cc(offsetof(KeyValue, name) == 20); +#define BAD_OFFSET(offset, len, max) \ + ((UINT64) (offset) + (len) >= (max)) + +#define BAD_STRUCT(type, offset, max) \ + ((UINT64) (offset) + sizeof(type) >= (max)) + +#define BAD_ARRAY(type, array, offset, array_len, max) \ + ((UINT64) (offset) + offsetof(type, array) + \ + sizeof((type){}.array[0]) * (UINT64) (array_len) >= (max)) + static const Key *get_key(const UINT8 *bcd, UINT32 bcd_len, UINT32 offset, const CHAR8 *name); static const Key *get_subkey(const UINT8 *bcd, UINT32 bcd_len, UINT32 offset, const CHAR8 *name) { assert(bcd); assert(name); - if ((UINT64) offset + sizeof(SubkeyFast) >= bcd_len) + if (BAD_STRUCT(SubkeyFast, offset, bcd_len)) return NULL; const SubkeyFast *subkey = (const SubkeyFast *) (bcd + offset); if (subkey->sig != SIG_SUBKEY_FAST) return NULL; - if ((UINT64) offset + offsetof(SubkeyFast, entries) + sizeof(struct SubkeyFastEntry[subkey->n_entries]) >= bcd_len) + if (BAD_ARRAY(SubkeyFast, entries, offset, subkey->n_entries, bcd_len)) return NULL; for (UINT16 i = 0; i < subkey->n_entries; i++) { @@ -147,14 +157,14 @@ static const Key *get_key(const UINT8 *bcd, UINT32 bcd_len, UINT32 offset, const assert(bcd); assert(name); - if ((UINT64) offset + sizeof(Key) >= bcd_len) + if (BAD_STRUCT(Key, offset, bcd_len)) return NULL; const Key *key = (const Key *) (bcd + offset); if (key->sig != SIG_KEY) return NULL; - if ((UINT64) offset + offsetof(Key, key_name) + sizeof(CHAR8[key->key_name_len]) >= bcd_len) + if (BAD_ARRAY(Key, key_name, offset, key->key_name_len, bcd_len)) return NULL; if (*name) { @@ -176,7 +186,7 @@ static const KeyValue *get_key_value(const UINT8 *bcd, UINT32 bcd_len, const Key if (key->n_key_values == 0) return NULL; - if ((UINT64) key->key_values_offset + sizeof(UINT32[key->n_key_values]) >= bcd_len || + if (BAD_OFFSET(key->key_values_offset, sizeof(UINT32) * (UINT64) key->n_key_values, bcd_len) || (UINTN)(bcd + key->key_values_offset) % sizeof(UINT32) != 0) return NULL; @@ -184,14 +194,14 @@ static const KeyValue *get_key_value(const UINT8 *bcd, UINT32 bcd_len, const Key for (UINT32 i = 0; i < key->n_key_values; i++) { UINT32 offset = *(key_value_list + i); - if ((UINT64) offset + sizeof(KeyValue) >= bcd_len) + if (BAD_STRUCT(KeyValue, offset, bcd_len)) continue; const KeyValue *kv = (const KeyValue *) (bcd + offset); if (kv->sig != SIG_KEY_VALUE) continue; - if ((UINT64) offset + offsetof(KeyValue, name) + kv->name_len >= bcd_len) + if (BAD_ARRAY(KeyValue, name, offset, kv->name_len, bcd_len)) continue; /* If most significant bit is set, data is stored in data_offset itself, but @@ -200,7 +210,7 @@ static const KeyValue *get_key_value(const UINT8 *bcd, UINT32 bcd_len, const Key if (FLAGS_SET(kv->data_size, UINT32_C(1) << 31)) continue; - if ((UINT64) kv->data_offset + kv->data_size >= bcd_len) + if (BAD_OFFSET(kv->data_offset, kv->data_size, bcd_len)) continue; if (strncaseeqa(name, kv->name, kv->name_len) && strlena(name) == kv->name_len)