From 9d30a941cce5ed055da18398f4deba18830d00d6 Mon Sep 17 00:00:00 2001 From: Asherah Connor Date: Tue, 9 Feb 2021 06:19:48 +0000 Subject: [PATCH 1/4] efi_loader: don't load beyond VirtualSize PE section table entries' SizeOfRawData must be a multiple of FileAlignment, and thus may be rounded up and larger than their VirtualSize. We should not load beyond the VirtualSize, which is "the total size of the section when loaded into memory" -- we may clobber real data at the target in some other section, since we load sections in reverse order and sections are usually laid out sequentially. Signed-off-by: Asherah Connor Reviewed-by: Heinrich Schuchardt --- lib/efi_loader/efi_image_loader.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index d4dd9e9433..f53ef367ec 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -843,7 +843,7 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, sec->Misc.VirtualSize); memcpy(efi_reloc + sec->VirtualAddress, efi + sec->PointerToRawData, - sec->SizeOfRawData); + min(sec->Misc.VirtualSize, sec->SizeOfRawData)); } /* Run through relocations */ From 841f7a4ebbe76c7843a864a5f9ca8f5f95a23df8 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 9 Feb 2021 17:45:33 +0100 Subject: [PATCH 2/4] efi_loader: '.' and '..' are directories '.' and '..' are directories. So when looking for capsule files it is sufficient to check that the attribute EFI_FILE_DIRECTORY is not set. We don't have to check for these special names. Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_capsule.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index 0d5a7b63ec..d39d731080 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -753,9 +753,7 @@ static efi_status_t efi_capsule_scan_dir(u16 ***files, unsigned int *num) if (!tmp_size) break; - if (!(dirent->attribute & EFI_FILE_DIRECTORY) && - u16_strcmp(dirent->file_name, L".") && - u16_strcmp(dirent->file_name, L"..")) + if (!(dirent->attribute & EFI_FILE_DIRECTORY)) count++; } From 15bbcafab1cb35614cd8d6fa52f0e90a894c1af5 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Tue, 9 Feb 2021 20:20:34 +0100 Subject: [PATCH 3/4] efi_loader: fix get_last_capsule() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix get_last_capsule() leads to writes beyond the stack allocated buffer. This was indicated when enabling the stack protector. utf16_utf8_strcpy() only stops copying when reaching '\0'. The current invocation always writes beyond the end of value[]. The output length of utf16_utf8_strcpy() may be longer than the number of UTF-16 tokens. E.g has "CapsuleКиев" has 11 UTF-16 tokens but 15 UTF-8 tokens. Hence, using utf16_utf8_strcpy() without checking the input may lead to further writes beyond value[]. The current invocation of strict_strtoul() reads beyond the end of value[]. A non-hexadecimal value after "Capsule" (e.g. "CapsuleZZZZ") must result in an error. We cat catch this by checking the return value of strict_strtoul(). A value that is too short after "Capsule" (e.g. "Capsule0") must result in an error. We must check the string length of value[]. Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_capsule.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c index d39d731080..b57f0302c5 100644 --- a/lib/efi_loader/efi_capsule.c +++ b/lib/efi_loader/efi_capsule.c @@ -42,20 +42,28 @@ static struct efi_file_handle *bootdev_root; static __maybe_unused unsigned int get_last_capsule(void) { u16 value16[11]; /* "CapsuleXXXX": non-null-terminated */ - char value[11], *p; + char value[5]; efi_uintn_t size; unsigned long index = 0xffff; efi_status_t ret; + int i; size = sizeof(value16); ret = efi_get_variable_int(L"CapsuleLast", &efi_guid_capsule_report, NULL, &size, value16, NULL); - if (ret != EFI_SUCCESS || u16_strncmp(value16, L"Capsule", 7)) + if (ret != EFI_SUCCESS || size != 22 || + u16_strncmp(value16, L"Capsule", 7)) goto err; + for (i = 0; i < 4; ++i) { + u16 c = value16[i + 7]; - p = value; - utf16_utf8_strcpy(&p, value16); - strict_strtoul(&value[7], 16, &index); + if (!c || c > 0x7f) + goto err; + value[i] = c; + } + value[4] = 0; + if (strict_strtoul(value, 16, &index)) + index = 0xffff; err: return index; } From fd434f47d4d008d41f4ee2fe5cb94791f780395c Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Thu, 11 Feb 2021 12:03:22 +0100 Subject: [PATCH 4/4] buildman: 'Thread' object has no attribute 'isAlive' The isAlive() method was deprecated in Python 3.8 and has been removed in Python 3.9. See https://bugs.python.org/issue37804. Use is_alive() instead. Since Python 2.6 is_alive() has been a synonym for isAlive(). So there should be no problems for users using elder Python 3 versions. Signed-off-by: Heinrich Schuchardt Reviewed-by: Simon Glass --- tools/buildman/builder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py index c93946842a..6f6d759329 100644 --- a/tools/buildman/builder.py +++ b/tools/buildman/builder.py @@ -1691,7 +1691,7 @@ class Builder: term = threading.Thread(target=self.queue.join) term.setDaemon(True) term.start() - while term.isAlive(): + while term.is_alive(): term.join(100) # Wait until we have processed all output