From fab82756462fd0ce82836e3d95721954d7ab2527 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Mon, 27 Dec 2021 22:41:50 +0100 Subject: [PATCH 1/3] boot: Fix off-by-one NUL-termination --- src/boot/efi/bcd.c | 2 +- src/boot/efi/util.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/boot/efi/bcd.c b/src/boot/efi/bcd.c index 970c8b1c8e..1f9f19ba63 100644 --- a/src/boot/efi/bcd.c +++ b/src/boot/efi/bcd.c @@ -316,6 +316,6 @@ TEST_STATIC CHAR16 *get_bcd_title(UINT8 *bcd, UINTN bcd_len) { /* The data should already be NUL-terminated. */ CHAR16 *title = (CHAR16 *) (bcd + description_value->data_offset); - title[description_value->data_size / sizeof(CHAR16)] = '\0'; + title[description_value->data_size / sizeof(CHAR16) - 1] = '\0'; return title; } diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index 6db4ab3969..76e4eef1eb 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -174,7 +174,7 @@ EFI_STATUS efivar_get(const EFI_GUID *vendor, const CHAR16 *name, CHAR16 **value return EFI_SUCCESS; /* Return buffer directly if it happens to be NUL terminated already */ - if (size >= sizeof(CHAR16) && buf[size/sizeof(CHAR16)] == 0) { + if (size >= sizeof(CHAR16) && buf[size / sizeof(CHAR16) - 1] == 0) { *value = TAKE_PTR(buf); return EFI_SUCCESS; } @@ -183,7 +183,7 @@ EFI_STATUS efivar_get(const EFI_GUID *vendor, const CHAR16 *name, CHAR16 **value val = xallocate_pool(size + sizeof(CHAR16)); CopyMem(val, buf, size); - val[size / sizeof(CHAR16)] = 0; /* NUL terminate */ + val[size / sizeof(CHAR16) - 1] = 0; /* NUL terminate */ *value = val; return EFI_SUCCESS; From c3c5b93a0c04c4940724b7babca92f4e75f49b98 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Mon, 27 Dec 2021 22:46:06 +0100 Subject: [PATCH 2/3] boot: Fix off-by-one offset sanity checks --- src/boot/efi/bcd.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/boot/efi/bcd.c b/src/boot/efi/bcd.c index 1f9f19ba63..07948392ff 100644 --- a/src/boot/efi/bcd.c +++ b/src/boot/efi/bcd.c @@ -117,14 +117,14 @@ static const Key *get_subkey(const UINT8 *bcd, UINT32 bcd_len, UINT32 offset, co assert(bcd); assert(name); - if ((UINT64) offset + sizeof(SubkeyFast) > bcd_len) + if ((UINT64) offset + sizeof(SubkeyFast) >= 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 ((UINT64) offset + offsetof(SubkeyFast, entries) + sizeof(struct SubkeyFastEntry[subkey->n_entries]) >= bcd_len) return NULL; for (UINT16 i = 0; i < subkey->n_entries; i++) { @@ -146,14 +146,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 ((UINT64) offset + sizeof(Key) >= 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 ((UINT64) offset + offsetof(Key, key_name) + sizeof(CHAR8[key->key_name_len]) >= bcd_len) return NULL; if (*name) { @@ -175,21 +175,21 @@ 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) return NULL; const UINT32 *key_value_list = (const UINT32 *) (bcd + key->key_values_offset); for (UINT32 i = 0; i < key->n_key_values; i++) { UINT32 offset = *(key_value_list + i); - if ((UINT64) offset + sizeof(KeyValue) > bcd_len) + if ((UINT64) offset + sizeof(KeyValue) >= 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 ((UINT64) offset + offsetof(KeyValue, name) + kv->name_len >= bcd_len) continue; /* If most significant bit is set, data is stored in data_offset itself, but @@ -198,7 +198,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 ((UINT64) kv->data_offset + kv->data_size >= bcd_len) continue; if (strncaseeqa(name, kv->name, kv->name_len) && !name[kv->name_len]) @@ -228,7 +228,7 @@ static const KeyValue *get_key_value(const UINT8 *bcd, UINT32 bcd_len, const Key TEST_STATIC CHAR16 *get_bcd_title(UINT8 *bcd, UINTN bcd_len) { assert(bcd); - if (HIVE_CELL_OFFSET > bcd_len) + if (HIVE_CELL_OFFSET >= bcd_len) return NULL; BaseBlock *base_block = (BaseBlock *) bcd; From 2198a773916f0e4ecca01725118f1f5a6bbe27b1 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Mon, 27 Dec 2021 22:49:02 +0100 Subject: [PATCH 3/3] boot: Fix name length comparison --- src/boot/efi/bcd.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/boot/efi/bcd.c b/src/boot/efi/bcd.c index 07948392ff..44c544f8f7 100644 --- a/src/boot/efi/bcd.c +++ b/src/boot/efi/bcd.c @@ -19,6 +19,7 @@ # define UINT32 uint32_t # define UINT64 uint64_t # define UINTN size_t +# define strlena(s) strlen(s) # define strncaseeqa(a, b, n) strncaseeq((a), (b), (n)) # define TEST_STATIC static #endif @@ -157,7 +158,7 @@ static const Key *get_key(const UINT8 *bcd, UINT32 bcd_len, UINT32 offset, const return NULL; if (*name) { - if (strncaseeqa(name, key->key_name, key->key_name_len) && !name[key->key_name_len]) + if (strncaseeqa(name, key->key_name, key->key_name_len) && strlena(name) == key->key_name_len) name += key->key_name_len; else return NULL; @@ -201,7 +202,7 @@ static const KeyValue *get_key_value(const UINT8 *bcd, UINT32 bcd_len, const Key if ((UINT64) kv->data_offset + kv->data_size >= bcd_len) continue; - if (strncaseeqa(name, kv->name, kv->name_len) && !name[kv->name_len]) + if (strncaseeqa(name, kv->name, kv->name_len) && strlena(name) == kv->name_len) return kv; }