From 0e69bcfb27644f9e7c575aa3565bcff922c7dec2 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sun, 31 May 2020 22:46:09 +0200 Subject: efi_loader: validate load option For passing the optional data of the load option to the loaded imaged protocol we need its size. efi_deserialize_load_option() is changed to return the size of the optional data. As a by-product we get a partial validation of the load option. Checking the length of the device path remains to be implemented. Some Coverity defects identified the load options as user input because get_unaligned_le32() and get_unaligned_le16() is called. But non of these Coverity defects can be resolved without marking functions with Coverity specific tags. Reported-by: Coverity (CID 303760) Reported-by: Coverity (CID 303768) Reported-by: Coverity (CID 303776) Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_bootmgr.c | 48 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 9 deletions(-) (limited to 'lib') diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index b112f5d..e144b3e 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -36,24 +36,50 @@ static const struct efi_runtime_services *rs; * * @lo: pointer to target * @data: serialized data + * @size: size of the load option, on return size of the optional data + * Return: status code */ -void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data) +efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data, + efi_uintn_t *size) { + efi_uintn_t len; + + len = sizeof(u32); + if (*size < len + 2 * sizeof(u16)) + return EFI_INVALID_PARAMETER; lo->attributes = get_unaligned_le32(data); - data += sizeof(u32); + data += len; + *size -= len; + len = sizeof(u16); lo->file_path_length = get_unaligned_le16(data); - data += sizeof(u16); + data += len; + *size -= len; - /* FIXME */ lo->label = (u16 *)data; - data += (u16_strlen(lo->label) + 1) * sizeof(u16); - - /* FIXME */ + len = u16_strnlen(lo->label, *size / sizeof(u16) - 1); + if (lo->label[len]) + return EFI_INVALID_PARAMETER; + len = (len + 1) * sizeof(u16); + if (*size < len) + return EFI_INVALID_PARAMETER; + data += len; + *size -= len; + + len = lo->file_path_length; + if (*size < len) + return EFI_INVALID_PARAMETER; lo->file_path = (struct efi_device_path *)data; - data += lo->file_path_length; + /* + * TODO: validate device path. There should be an end node within + * the indicated file_path_length. + */ + data += len; + *size -= len; lo->optional_data = data; + + return EFI_SUCCESS; } /** @@ -168,7 +194,11 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle) if (!load_option) return EFI_LOAD_ERROR; - efi_deserialize_load_option(&lo, load_option); + ret = efi_deserialize_load_option(&lo, load_option, &size); + if (ret != EFI_SUCCESS) { + log_warning("Invalid load option for %ls\n", varname); + goto error; + } if (lo.attributes & LOAD_OPTION_ACTIVE) { u32 attributes; -- cgit v1.1 From 4f7dc5f608c07f5bc169217bedac6883b0d998df Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Wed, 27 May 2020 01:58:30 +0200 Subject: efi_loader: allow compiling with clang On ARM systems gd is stored in register r9 or x18. When compiling with clang gd is defined as a macro calling function gd_ptr(). So we can not make assignments to gd. In the UEFI sub-system we need to save gd when leaving to UEFI binaries and have to restore gd when reentering U-Boot. Define a new function set_gd() for setting gd and use it in the UEFI sub-system. Signed-off-by: Heinrich Schuchardt Tested-by: Tom Rini --- lib/efi_loader/efi_boottime.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'lib') diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index db34938..1591ad8 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -49,7 +49,7 @@ static efi_handle_t current_image; * restriction so we need to manually swap its and our view of that register on * EFI callback entry/exit. */ -static volatile void *efi_gd, *app_gd; +static volatile gd_t *efi_gd, *app_gd; #endif /* 1 if inside U-Boot code, 0 if inside EFI payload code */ @@ -89,7 +89,7 @@ int __efi_entry_check(void) #ifdef CONFIG_ARM assert(efi_gd); app_gd = gd; - gd = efi_gd; + set_gd(efi_gd); #endif return ret; } @@ -99,7 +99,7 @@ int __efi_exit_check(void) { int ret = --entry_count == 0; #ifdef CONFIG_ARM - gd = app_gd; + set_gd(app_gd); #endif return ret; } @@ -123,7 +123,7 @@ void efi_restore_gd(void) /* Only restore if we're already in EFI context */ if (!efi_gd) return; - gd = efi_gd; + set_gd(efi_gd); #endif } @@ -2920,7 +2920,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, * otherwise __efi_entry_check() will put the wrong value into * app_gd. */ - gd = app_gd; + set_gd(app_gd); #endif /* * To get ready to call EFI_EXIT below we have to execute the -- cgit v1.1 From 4afceb4d17ecb07ff92c8489fea066a288e1030e Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 30 May 2020 05:48:08 +0200 Subject: efi_loader: function descriptions efi_image_loader.c We want to follow the Linux kernel style for function descriptions. Add missing parentheses after function names. Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_image_loader.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 5dd6019..ac7ea18 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -212,7 +212,7 @@ static void efi_set_code_and_data_type( #ifdef CONFIG_EFI_SECURE_BOOT /** - * cmp_pe_section - compare two sections + * cmp_pe_section() - compare two sections * @arg1: Pointer to pointer to first section * @arg2: Pointer to pointer to second section * @@ -237,7 +237,7 @@ static int cmp_pe_section(const void *arg1, const void *arg2) } /** - * efi_image_parse - parse a PE image + * efi_image_parse() - parse a PE image * @efi: Pointer to image * @len: Size of @efi * @regp: Pointer to a list of regions @@ -404,7 +404,7 @@ err: } /** - * efi_image_unsigned_authenticate - authenticate unsigned image with + * efi_image_unsigned_authenticate() - authenticate unsigned image with * SHA256 hash * @regs: List of regions to be verified * @@ -451,7 +451,7 @@ out: } /** - * efi_image_authenticate - verify a signature of signed image + * efi_image_authenticate() - verify a signature of signed image * @efi: Pointer to image * @efi_size: Size of @efi * -- cgit v1.1 From 13f62d9f7edf4fe6a63506e83df03f43a9041cba Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 30 May 2020 06:44:48 +0200 Subject: efi_loader: function description cmp_pe_section() Rework the description of function cmp_pe_section(). Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_image_loader.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'lib') diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index ac7ea18..c273287 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -212,14 +212,16 @@ static void efi_set_code_and_data_type( #ifdef CONFIG_EFI_SECURE_BOOT /** - * cmp_pe_section() - compare two sections - * @arg1: Pointer to pointer to first section - * @arg2: Pointer to pointer to second section + * cmp_pe_section() - compare virtual addresses of two PE image sections + * @arg1: pointer to pointer to first section header + * @arg2: pointer to pointer to second section header * - * Compare two sections in PE image. + * Compare the virtual addresses of two sections of an portable executable. + * The arguments are defined as const void * to allow usage with qsort(). * - * Return: -1, 0, 1 respectively if arg1 < arg2, arg1 == arg2 or - * arg1 > arg2 + * Return: -1 if the virtual address of arg1 is less than that of arg2, + * 0 if the virtual addresses are equal, 1 if the virtual address + * of arg1 is greater than that of arg2. */ static int cmp_pe_section(const void *arg1, const void *arg2) { -- cgit v1.1 From 55af40a5781f1de574822745c9c76b3a928388cd Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 30 May 2020 07:35:59 +0200 Subject: efi_loader: simplify PE consistency check Knowing that at least one section header follows the optional header we only need to check for the length of the 64bit header which is longer than the 32bit header. Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_image_loader.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) (limited to 'lib') diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index c273287..478aaf5 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -637,21 +637,18 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, goto err; } - /* assume sizeof(IMAGE_NT_HEADERS32) <= sizeof(IMAGE_NT_HEADERS64) */ - if (efi_size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS32)) { + /* + * Check if the image section header fits into the file. Knowing that at + * least one section header follows we only need to check for the length + * of the 64bit header which is longer than the 32bit header. + */ + if (efi_size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS64)) { printf("%s: Invalid offset for Extended Header\n", __func__); ret = EFI_LOAD_ERROR; goto err; } nt = (void *) ((char *)efi + dos->e_lfanew); - if ((nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) && - (efi_size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS64))) { - printf("%s: Invalid offset for Extended Header\n", __func__); - ret = EFI_LOAD_ERROR; - goto err; - } - if (nt->Signature != IMAGE_NT_SIGNATURE) { printf("%s: Invalid NT Signature\n", __func__); ret = EFI_LOAD_ERROR; -- cgit v1.1 From a4292eccfdc98b51d0200a6c912af237aeddd5c8 Mon Sep 17 00:00:00 2001 From: AKASHI Takahiro Date: Fri, 29 May 2020 15:41:18 +0900 Subject: efi_loader: signature: move efi_guid_cert_type_pkcs7 to efi_signature.c The global variable, efi_guid_cert_type_pkcs7, will also be used in efi_image_loader.c in a succeeding patch so as to correctly handle a signature type of authenticode in signed image. Meanwhile, it is currently defined in efi_variable.c. Once some secure storage solution for UEFI variables is introduced, efi_variable.c may not always be compiled in. So move the definition to efi_signature.c as a common place. Signed-off-by: AKASHI Takahiro Reviewed-by: Heinrich Schuchardt --- lib/efi_loader/efi_signature.c | 1 + lib/efi_loader/efi_variable.c | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c index adcb8c9..6685253 100644 --- a/lib/efi_loader/efi_signature.c +++ b/lib/efi_loader/efi_signature.c @@ -22,6 +22,7 @@ const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID; const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID; const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID; const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID; +const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; #ifdef CONFIG_EFI_SECURE_BOOT diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 0a43db5..e097670 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -26,7 +26,6 @@ enum efi_secure_mode { EFI_MODE_DEPLOYED, }; -const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID; static bool efi_secure_boot; static int efi_secure_mode; static u8 efi_vendor_keys; -- cgit v1.1