From f0b0f7fe0eecde78a6e3e0f6760834ff12642375 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Thu, 19 Mar 2020 17:15:18 +0000 Subject: [PATCH 1/9] efi_loader: description of efi_variable.c Correct the file description. Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_variable.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index c316bdfec0e..99d2f01f574 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -1,8 +1,8 @@ // SPDX-License-Identifier: GPL-2.0+ /* - * EFI utils + * UEFI runtime variable services * - * Copyright (c) 2017 Rob Clark + * Copyright (c) 2017 Rob Clark */ #include From 47a9596354e074146596559ee7330e1941db43be Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Thu, 19 Mar 2020 13:49:34 +0100 Subject: [PATCH 2/9] efi_loader: fix function descriptions in efi_disk.c Use Sphinx style for function descriptions. Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_disk.c | 52 ++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index ed7fb3f7d33..9563556691b 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -222,15 +222,17 @@ static const struct efi_block_io block_io_disk_template = { .flush_blocks = &efi_disk_flush_blocks, }; -/* - * Get the simple file system protocol for a file device path. +/** + * efi_fs_from_path() - retrieve simple file system protocol + * + * Gets the simple file system protocol for a file device path. * * The full path provided is split into device part and into a file * part. The device part is used to find the handle on which the * simple file system protocol is installed. * - * @full_path device path including device and file - * @return simple file system protocol + * @full_path: device path including device and file + * Return: simple file system protocol */ struct efi_simple_file_system_protocol * efi_fs_from_path(struct efi_device_path *full_path) @@ -285,15 +287,15 @@ static int efi_fs_exists(struct blk_desc *desc, int part) } /* - * Create a handle for a partition or disk + * efi_disk_add_dev() - create a handle for a partition or disk * - * @parent parent handle - * @dp_parent parent device path - * @if_typename interface name for block device - * @desc internal block device - * @dev_index device index for block device - * @offset offset into disk for simple partitions - * @return disk object + * @parent: parent handle + * @dp_parent: parent device path + * @if_typename: interface name for block device + * @desc: internal block device + * @dev_index: device index for block device + * @offset: offset into disk for simple partitions + * Return: disk object */ static efi_status_t efi_disk_add_dev( efi_handle_t parent, @@ -373,15 +375,17 @@ static efi_status_t efi_disk_add_dev( return EFI_SUCCESS; } -/* - * Create handles and protocols for the partitions of a block device +/** + * efi_disk_create_partitions() - create handles and protocols for partitions * - * @parent handle of the parent disk - * @blk_desc block device - * @if_typename interface type - * @diskid device number - * @pdevname device name - * @return number of partitions created + * Create handles and protocols for the partitions of a block device. + * + * @parent: handle of the parent disk + * @blk_desc: block device + * @if_typename: interface type + * @diskid: device number + * @pdevname: device name + * Return: number of partitions created */ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, const char *if_typename, int diskid, @@ -418,16 +422,20 @@ int efi_disk_create_partitions(efi_handle_t parent, struct blk_desc *desc, return disks; } -/* +/** + * efi_disk_register() - register block devices + * * U-Boot doesn't have a list of all online disk devices. So when running our * EFI payload, we scan through all of the potentially available ones and * store them in our object pool. * + * This function is called in efi_init_obj_list(). + * * TODO(sjg@chromium.org): Actually with CONFIG_BLK, U-Boot does have this. * Consider converting the code to look up devices as needed. The EFI device * could be a child of the UCLASS_BLK block device, perhaps. * - * This gets called from do_bootefi_exec(). + * Return: status code */ efi_status_t efi_disk_register(void) { From 4d7f5af841c4622fb6c5d155e31c1072f3b052df Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Fri, 20 Mar 2020 19:04:34 +0100 Subject: [PATCH 3/9] efi_loader: correct reported length in GetNextVariable() The runtime service GetNextVariable() returns the length of the next variable including the closing 0x0000. This length should be in bytes. Comparing the output of EDK2 and U-Boot shows that this is currently not correctly implemented: EDK2: OsIndicationsSupported: 46 PlatformLang: 26 PlatformLangCodes: 36 U-Boot: OsIndicationsSupported: 23 PlatformLang: 13 PlatformLangCodes: 18 Provide correct length in GetNextVariable(). Fixes: d99a87f84b75 ("efi_loader: implement GetNextVariableName()") Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_variable.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 99d2f01f574..3bec2d0d17a 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -273,7 +273,8 @@ static efi_status_t parse_uboot_variable(char *variable, u32 *attributes) { char *guid, *name, *end, c; - unsigned long name_len; + size_t name_len; + efi_uintn_t old_variable_name_size; u16 *p; guid = strchr(variable, '_'); @@ -289,17 +290,17 @@ static efi_status_t parse_uboot_variable(char *variable, return EFI_INVALID_PARAMETER; name_len = end - name; - if (*variable_name_size < (name_len + 1)) { - *variable_name_size = name_len + 1; + old_variable_name_size = *variable_name_size; + *variable_name_size = sizeof(u16) * (name_len + 1); + if (old_variable_name_size < *variable_name_size) return EFI_BUFFER_TOO_SMALL; - } + end++; /* point to value */ /* variable name */ p = variable_name; utf8_utf16_strncpy(&p, name, name_len); variable_name[name_len] = 0; - *variable_name_size = name_len + 1; /* guid */ c = *(name - 1); From e1089765b5964f35a426f3abe29ba164422f4165 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Fri, 20 Mar 2020 19:20:17 +0100 Subject: [PATCH 4/9] efi_selftest: check length reported by GetNextVariableName() GetNextVariableName should report the length of the variable including the final 0x0000 in bytes. Check this in the unit test. Increase the buffer size for variable names. 40 bytes is too short. Signed-off-by: Heinrich Schuchardt --- lib/efi_selftest/efi_selftest_variables.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/efi_selftest/efi_selftest_variables.c b/lib/efi_selftest/efi_selftest_variables.c index 5d98c029b86..2c16f3db6cc 100644 --- a/lib/efi_selftest/efi_selftest_variables.c +++ b/lib/efi_selftest/efi_selftest_variables.c @@ -11,7 +11,7 @@ #include #define EFI_ST_MAX_DATA_SIZE 16 -#define EFI_ST_MAX_VARNAME_SIZE 40 +#define EFI_ST_MAX_VARNAME_SIZE 80 static struct efi_boot_services *boottime; static struct efi_runtime_services *runtime; @@ -155,8 +155,14 @@ static int execute(void) return EFI_ST_FAILURE; } if (!memcmp(&guid, &guid_vendor0, sizeof(efi_guid_t)) && - !efi_st_strcmp_16_8(varname, "efi_st_var0")) + !efi_st_strcmp_16_8(varname, "efi_st_var0")) { flag |= 1; + if (len != 24) { + efi_st_error("GetNextVariableName report wrong length %u, expected 24\n", + (unsigned int)len); + return EFI_ST_FAILURE; + } + } if (!memcmp(&guid, &guid_vendor1, sizeof(efi_guid_t)) && !efi_st_strcmp_16_8(varname, "efi_st_var1")) flag |= 2; From 9f888969fdd6ef11078b893639352ec57824c202 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Thu, 19 Mar 2020 15:45:52 +0100 Subject: [PATCH 5/9] efi_loader: simplify logical expression in efi_disk_add_dev() To check if a variable is non-zero there is no need for '!= 0'. Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_disk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index 9563556691b..fc0682bc48c 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -367,7 +367,7 @@ static efi_status_t efi_disk_add_dev( diskobj->media.block_size = desc->blksz; diskobj->media.io_align = desc->blksz; diskobj->media.last_block = desc->lba - offset; - if (part != 0) + if (part) diskobj->media.logical_partition = 1; diskobj->ops.media = &diskobj->media; if (disk) From 7aeceffb2509ba700673fa125bbfe785c4c0be71 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sun, 22 Mar 2020 08:28:15 +0100 Subject: [PATCH 6/9] efi_loader: description efi_convert_pointer() Correct the description of function efi_convert_pointer(). Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_runtime.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c index 4be51335bcb..6a25acbbcdf 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -483,7 +483,7 @@ static __efi_runtime efi_status_t EFIAPI efi_convert_pointer_runtime( } /** - * efi_convert_pointer_runtime() - convert from physical to virtual pointer + * efi_convert_pointer() - convert from physical to virtual pointer * * This function implements the ConvertPointer() runtime service until * the first call to SetVirtualAddressMap(). @@ -493,7 +493,7 @@ static __efi_runtime efi_status_t EFIAPI efi_convert_pointer_runtime( * * @debug_disposition: indicates if pointer may be converted to NULL * @address: pointer to be converted - * Return: status code EFI_UNSUPPORTED + * Return: status code */ static __efi_runtime efi_status_t EFIAPI efi_convert_pointer( efi_uintn_t debug_disposition, void **address) From 72291a9d83ec2fe50cc8fac304e8ecd5534daf5e Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sun, 22 Mar 2020 09:52:48 +0100 Subject: [PATCH 7/9] efi_loader: fix freestanding memmove() For EFI binaries we have to provide an implementation of memmove() in efi_freestanding.c. Before this patch the memmove() function was copying in the wrong direction. Signed-off-by: Heinrich Schuchardt --- lib/efi_loader/efi_freestanding.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/efi_loader/efi_freestanding.c b/lib/efi_loader/efi_freestanding.c index dcf5d1c49a2..bd0dff162f6 100644 --- a/lib/efi_loader/efi_freestanding.c +++ b/lib/efi_loader/efi_freestanding.c @@ -47,7 +47,7 @@ void *memmove(void *dest, const void *src, size_t n) u8 *d = dest; const u8 *s = src; - if (d >= s) { + if (d <= s) { for (; n; --n) *d++ = *s++; } else { From cde162e76680c57e9756d937ff23c822249bc3af Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sun, 22 Mar 2020 09:32:55 +0100 Subject: [PATCH 8/9] efi_selftest: test CalculateCrc32, CopyMem, SetMem Provide unit tests for CalculateCrc32(), CopyMem(), SetMem(). Signed-off-by: Heinrich Schuchardt --- lib/efi_selftest/Makefile | 1 + lib/efi_selftest/efi_selftest_mem.c | 77 +++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) create mode 100644 lib/efi_selftest/efi_selftest_mem.c diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile index cf132c372e1..e9baa641350 100644 --- a/lib/efi_selftest/Makefile +++ b/lib/efi_selftest/Makefile @@ -27,6 +27,7 @@ efi_selftest_exitbootservices.o \ efi_selftest_gop.o \ efi_selftest_loaded_image.o \ efi_selftest_manageprotocols.o \ +efi_selftest_mem.o \ efi_selftest_memory.o \ efi_selftest_open_protocol.o \ efi_selftest_register_notify.o \ diff --git a/lib/efi_selftest/efi_selftest_mem.c b/lib/efi_selftest/efi_selftest_mem.c new file mode 100644 index 00000000000..51f0fec39b9 --- /dev/null +++ b/lib/efi_selftest/efi_selftest_mem.c @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * efi_selftest_memory + * + * Copyright (c) 2018 Heinrich Schuchardt + * + * This unit test checks the following boottime services: + * CopyMem, SetMem, CalculateCrc32 + * + * The memory type used for the device tree is checked. + */ + +#include + +static struct efi_boot_services *boottime; + +/** + * setup() - setup unit test + * + * @handle: handle of the loaded image + * @systable: system table + * Return: EFI_ST_SUCCESS for success + */ +static int setup(const efi_handle_t handle, + const struct efi_system_table *systable) +{ + boottime = systable->boottime; + + return EFI_ST_SUCCESS; +} + +/* + * execute() - execute unit test + * + * Return: EFI_ST_SUCCESS for success + */ +static int execute(void) +{ + u8 c1[] = "abcdefghijklmnop"; + u8 c2[] = "abcdefghijklmnop"; + u32 crc32; + efi_status_t ret; + + ret = boottime->calculate_crc32(c1, 16, &crc32); + if (ret != EFI_SUCCESS) { + efi_st_error("CalculateCrc32 failed\n"); + return EFI_ST_FAILURE; + } + if (crc32 != 0x943ac093) { + efi_st_error("CalculateCrc32 returned wrong value\n"); + return EFI_ST_FAILURE; + } + boottime->copy_mem(&c1[5], &c1[3], 8); + if (memcmp(c1, "abcdedefghijknop", 16)) { + efi_st_error("CopyMem forward copy failed: %s\n", c1); + return EFI_ST_FAILURE; + } + boottime->copy_mem(&c2[3], &c2[5], 8); + if (memcmp(c2, "abcfghijklmlmnop", 16)) { + efi_st_error("CopyMem backward copy failed: %s\n", c2); + return EFI_ST_FAILURE; + } + boottime->set_mem(&c1[3], 8, 'x'); + if (memcmp(c1, "abcxxxxxxxxjknop", 16)) { + efi_st_error("SetMem failed: %s\n", c1); + return EFI_ST_FAILURE; + } + + return EFI_ST_SUCCESS; +} + +EFI_UNIT_TEST(mem) = { + .name = "mem", + .phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT, + .setup = setup, + .execute = execute, +}; From 7a4e717b9c0c255137a58f3ab90f002fc3aade2b Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sun, 22 Mar 2020 18:28:20 +0100 Subject: [PATCH 9/9] efi_loader: definition of GetNextVariableName() 'vendor' is both an input and an output parameter. So it cannot be constant. Fixes: 0bda81bfdc5c ("efi_loader: use const efi_guid_t * for variable services") Signed-off-by: Heinrich Schuchardt --- include/efi_api.h | 2 +- include/efi_loader.h | 2 +- lib/efi_loader/efi_variable.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/efi_api.h b/include/efi_api.h index 4713da2e1d0..1c40ffc4f56 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -272,7 +272,7 @@ struct efi_runtime_services { efi_uintn_t *data_size, void *data); efi_status_t (EFIAPI *get_next_variable_name)( efi_uintn_t *variable_name_size, - u16 *variable_name, const efi_guid_t *vendor); + u16 *variable_name, efi_guid_t *vendor); efi_status_t (EFIAPI *set_variable)(u16 *variable_name, const efi_guid_t *vendor, u32 attributes, diff --git a/include/efi_loader.h b/include/efi_loader.h index 37c3f15da1b..3f2792892f3 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -645,7 +645,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *variable_name, efi_uintn_t *data_size, void *data); efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, u16 *variable_name, - const efi_guid_t *vendor); + efi_guid_t *vendor); efi_status_t EFIAPI efi_set_variable(u16 *variable_name, const efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, const void *data); diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 3bec2d0d17a..fe2f2645913 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -330,7 +330,7 @@ static efi_status_t parse_uboot_variable(char *variable, */ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size, u16 *variable_name, - const efi_guid_t *vendor) + efi_guid_t *vendor) { char *native_name, *variable; ssize_t name_len, list_len; @@ -598,7 +598,7 @@ efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *vendor, */ static efi_status_t __efi_runtime EFIAPI efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size, - u16 *variable_name, const efi_guid_t *vendor) + u16 *variable_name, efi_guid_t *vendor) { return EFI_UNSUPPORTED; }