From 52d7bfe78787c93b95e805b44bb4d746a65edde4 Mon Sep 17 00:00:00 2001 From: AKASHI Takahiro Date: Fri, 8 May 2020 14:51:59 +0900 Subject: [PATCH 01/10] efi_loader: image_loader: fix a Coverity check against array access Coverity detected: Using "&opt->CheckSum" as an array. This might corrupt or misinterpret adjacent memory locations. The code should work as far as a structure, IMAGE_OPTIONAL_HEADER(64) is packed, but modify it in more logical form. Subsystem is a member next to CheckSum. Reported-by: Coverity (CID 300339) Signed-off-by: AKASHI Takahiro Reviewed-by: Heinrich Schuchardt --- lib/efi_loader/efi_image_loader.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index 4e075ae416..5dd601908d 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -293,12 +293,12 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp, efi_image_region_add(regs, efi, &opt->CheckSum, 0); if (nt64->OptionalHeader.NumberOfRvaAndSizes <= ctidx) { efi_image_region_add(regs, - &opt->CheckSum + 1, + &opt->Subsystem, efi + opt->SizeOfHeaders, 0); } else { /* Skip Certificates Table */ efi_image_region_add(regs, - &opt->CheckSum + 1, + &opt->Subsystem, &opt->DataDirectory[ctidx], 0); efi_image_region_add(regs, &opt->DataDirectory[ctidx] + 1, @@ -313,7 +313,7 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp, IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader; efi_image_region_add(regs, efi, &opt->CheckSum, 0); - efi_image_region_add(regs, &opt->CheckSum + 1, + efi_image_region_add(regs, &opt->Subsystem, &opt->DataDirectory[ctidx], 0); efi_image_region_add(regs, &opt->DataDirectory[ctidx] + 1, efi + opt->SizeOfHeaders, 0); From b433acbb819e1546ef9493dc3ba97a44b398a9db Mon Sep 17 00:00:00 2001 From: AKASHI Takahiro Date: Fri, 8 May 2020 14:51:21 +0900 Subject: [PATCH 02/10] efi_loader: variable: check a return value of uuid__str_to_bin() The only error case is that a given UUID is in wrong format. So just return EFI_INVALID_PARAMETER here. Reported-by: Coverity (CID 300333) Signed-off-by: AKASHI Takahiro Reviewed-by: Heinrich Schuchardt --- lib/efi_loader/efi_variable.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 60c1201757..10892684d1 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -767,7 +767,10 @@ static efi_status_t parse_uboot_variable(char *variable, /* guid */ c = *(name - 1); *(name - 1) = '\0'; /* guid need be null-terminated here */ - uuid_str_to_bin(guid, (unsigned char *)vendor, UUID_STR_FORMAT_GUID); + if (uuid_str_to_bin(guid, (unsigned char *)vendor, + UUID_STR_FORMAT_GUID)) + /* The only error would be EINVAL. */ + return EFI_INVALID_PARAMETER; *(name - 1) = c; /* attributes */ From d67591dc22f9a6d41163fb6ba0efce5fa0598830 Mon Sep 17 00:00:00 2001 From: AKASHI Takahiro Date: Fri, 8 May 2020 14:50:47 +0900 Subject: [PATCH 03/10] cmd: efidebug: fix a wrong handling of arguments Coverity detected a dead code, but actually there is a bug in a check against a number of arguments. So simply fix it. Reported-by: Coverity (CID 300330) Signed-off-by: AKASHI Takahiro Reviewed-by: Heinrich Schuchardt --- cmd/efidebug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/efidebug.c b/cmd/efidebug.c index d8a76d78a3..5e3bf16573 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -602,7 +602,7 @@ static int do_efi_boot_add(cmd_tbl_t *cmdtp, int flag, + sizeof(struct efi_device_path); /* for END */ /* optional data */ - if (argc < 6) + if (argc == 6) lo.optional_data = NULL; else lo.optional_data = (const u8 *)argv[6]; From e2a5b8603087bc3f8f9acd4cc171df48e7ef0a6a Mon Sep 17 00:00:00 2001 From: AKASHI Takahiro Date: Fri, 8 May 2020 14:50:18 +0900 Subject: [PATCH 04/10] cmd: efidebug: add a comment against Coverity check (300329) The check here, "Null pointer dereferences," is a false positive. So leave a comment. Signed-off-by: AKASHI Takahiro Reported-by: Coverity (CID 300329) --- cmd/efidebug.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 5e3bf16573..668e5c4e87 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -480,6 +480,11 @@ static int do_efi_show_memmap(cmd_tbl_t *cmdtp, int flag, EFI_PHYS_ADDR_WIDTH - 5, spc, EFI_PHYS_ADDR_WIDTH - 3, spc); printf("================ %.*s %.*s ==========\n", EFI_PHYS_ADDR_WIDTH, sep, EFI_PHYS_ADDR_WIDTH, sep); + /* + * Coverity check: dereferencing null pointer "map." + * This is a false positive as memmap will always be + * populated by allocate_pool() above. + */ for (i = 0, map = memmap; i < map_size / sizeof(*map); map++, i++) { if (map->type < ARRAY_SIZE(efi_mem_type_string)) type = efi_mem_type_string[map->type]; From 7fec249bb76db8bc21b98c08822116597154704d Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 9 May 2020 07:33:43 +0200 Subject: [PATCH 05/10] efi_selftest: add unit test functions to HTML documentation Add the UEFI unit test helper functions to the generated HTML documentation. Correct some documentation texts in include/efi_selftest.h. Signed-off-by: Heinrich Schuchardt --- doc/api/efi.rst | 9 ++++ include/efi_selftest.h | 110 +++++++++++++++++++++++++++-------------- 2 files changed, 81 insertions(+), 38 deletions(-) diff --git a/doc/api/efi.rst b/doc/api/efi.rst index 0667c3aef7..d5114f05b3 100644 --- a/doc/api/efi.rst +++ b/doc/api/efi.rst @@ -163,3 +163,12 @@ Unicode Collation protocol .. kernel-doc:: lib/efi_loader/efi_unicode_collation.c :internal: + +Unit testing +------------ + +The following library functions are provided to support writing UEFI unit tests. +The should not be used elsewhere. + +.. kernel-doc:: include/efi_selftest.h + :internal: diff --git a/include/efi_selftest.h b/include/efi_selftest.h index eaee188de7..1515fdaa02 100644 --- a/include/efi_selftest.h +++ b/include/efi_selftest.h @@ -17,69 +17,89 @@ #define EFI_ST_SUCCESS 0 #define EFI_ST_FAILURE 1 #define EFI_ST_SUCCESS_STR L"SUCCESS" -/* - * Prints a message. + +/** + * efi_st_printf() - print a message + * + * @...: format string followed by fields to print */ #define efi_st_printf(...) \ (efi_st_printc(-1, __VA_ARGS__)) -/* - * Prints an error message. +/** + * efi_st_error() - prints an error message * - * @... format string followed by fields to print + * @...: format string followed by fields to print */ #define efi_st_error(...) \ (efi_st_printc(EFI_LIGHTRED, "%s(%u):\nERROR: ", __FILE__, __LINE__), \ efi_st_printc(EFI_LIGHTRED, __VA_ARGS__)) -/* - * Prints a TODO message. +/** + * efi_st_todo() - prints a TODO message * - * @... format string followed by fields to print + * @...: format string followed by fields to print */ #define efi_st_todo(...) \ (efi_st_printc(EFI_YELLOW, "%s(%u):\nTODO: ", __FILE__, __LINE__), \ efi_st_printc(EFI_YELLOW, __VA_ARGS__)) \ -/* +/** + * enum efi_test_phase - phase when test will be executed + * * A test may be setup and executed at boottime, * it may be setup at boottime and executed at runtime, * or it may be setup and executed at runtime. */ enum efi_test_phase { + /** + * @EFI_EXECUTE_BEFORE_BOOTTIME_EXIT: - execute before ExitBootServices + * + * Setup, execute, and teardown are executed before ExitBootServices(). + */ EFI_EXECUTE_BEFORE_BOOTTIME_EXIT = 1, + /** + * @EFI_SETUP_BEFORE_BOOTTIME_EXIT: - setup before ExitBootServices + * + * Setup is executed before ExitBootServices() while execute, and + * teardown are executed after ExitBootServices(). + */ EFI_SETUP_BEFORE_BOOTTIME_EXIT, + /** + * @EFI_SETUP_AFTER_BOOTTIME_EXIT: - setup after ExitBootServices + * + * Setup, execute, and teardown are executed after ExitBootServices(). + */ EFI_SETUP_AFTER_BOOTTIME_EXIT, }; extern struct efi_simple_text_output_protocol *con_out; extern struct efi_simple_text_input_protocol *con_in; -/* - * Exit the boot services. +/** + * efi_st_exit_boot_services() - exit the boot services * - * The size of the memory map is determined. - * Pool memory is allocated to copy the memory map. - * The memory amp is copied and the map key is obtained. - * The map key is used to exit the boot services. + * * The size of the memory map is determined. + * * Pool memory is allocated to copy the memory map. + * * The memory map is copied and the map key is obtained. + * * The map key is used to exit the boot services. */ void efi_st_exit_boot_services(void); -/* - * Print a colored message +/** + * efi_st_printc() - print a colored message * - * @color color, see constants in efi_api.h, use -1 for no color - * @fmt printf format - * @... arguments to be printed - * on return position of terminating zero word + * @color: color, see constants in efi_api.h, use -1 for no color + * @fmt: printf style format string + * @...: arguments to be printed */ void efi_st_printc(int color, const char *fmt, ...) __attribute__ ((format (__printf__, 2, 3))); /** - * efi_st_translate_char() - translate a unicode character to a string + * efi_st_translate_char() - translate a Unicode character to a string * - * @code: unicode character + * @code: Unicode character * Return: string */ u16 *efi_st_translate_char(u16 code); @@ -87,38 +107,44 @@ u16 *efi_st_translate_char(u16 code); /** * efi_st_translate_code() - translate a scan code to a human readable string * - * @code: unicode character - * Return: string + * This function translates the scan code returned by the simple text input + * protocol to a human readable string, e.g. 0x04 is translated to L"Left". + * + * @code: scan code + * Return: Unicode string */ u16 *efi_st_translate_code(u16 code); -/* - * Compare an u16 string to a char string. +/** + * efi_st_strcmp_16_8() - compare an u16 string to a char string + * + * This function compares each u16 value to the char value at the same + * position. This function is only useful for ANSI strings. * * @buf1: u16 string * @buf2: char string - * @return: 0 if both buffers contain the same bytes + * Return: 0 if both buffers contain equivalent strings */ int efi_st_strcmp_16_8(const u16 *buf1, const char *buf2); -/* - * Reads an Unicode character from the input device. +/** + * efi_st_get_key() - reads an Unicode character from the input device * - * @return: Unicode character + * Return: Unicode character */ u16 efi_st_get_key(void); /** * struct efi_unit_test - EFI unit test * - * An efi_unit_test provides a interface to an EFI unit test. + * The &struct efi_unit_test structure provides a interface to an EFI unit test. * - * @name: name of unit test + * @name: name of the unit test used in the user interface * @phase: specifies when setup and execute are executed - * @setup: set up the unit test - * @teardown: tear down the unit test - * @execute: execute the unit test - * @on_request: test is only executed on request + * @setup: set up function of the unit test + * @execute: execute function of the unit test + * @teardown: tear down function of the unit test + * @on_request: flag indicating that the test shall only be executed on request */ struct efi_unit_test { const char *name; @@ -130,7 +156,15 @@ struct efi_unit_test { bool on_request; }; -/* Declare a new EFI unit test */ +/** + * EFI_UNIT_TEST() - macro to declare a new EFI unit test + * + * The macro EFI_UNIT_TEST() declares an EFI unit test using the &struct + * efi_unit_test structure. The test is added to a linker generated list which + * is evaluated by the 'bootefi selftest' command. + * + * @__name: string identifying the unit test in the linker generated list + */ #define EFI_UNIT_TEST(__name) \ ll_entry_declare(struct efi_unit_test, __name, efi_unit_test) From 4835d35acfabe9fed464602fb919279036a49236 Mon Sep 17 00:00:00 2001 From: Sughosh Ganu Date: Wed, 6 May 2020 22:12:41 +0300 Subject: [PATCH 06/10] charset: Add support for calculating bytes occupied by a u16 string The current code uses 'u16_strlen(x) + 1) * sizeof(u16)' in various places to calculate the number of bytes occupied by a u16 string. Let's introduce a wrapper around this. This wrapper is used on following patches Signed-off-by: Sughosh Ganu Reviewed-by: Heinrich Schuchardt --- include/charset.h | 12 ++++++++++++ lib/charset.c | 5 +++++ 2 files changed, 17 insertions(+) diff --git a/include/charset.h b/include/charset.h index fde6bddbc2..c118cbeb9a 100644 --- a/include/charset.h +++ b/include/charset.h @@ -195,6 +195,18 @@ int u16_strncmp(const u16 *s1, const u16 *s2, size_t n); */ size_t u16_strlen(const void *in); +/** + * u16_strsize() - count size of u16 string in bytes including the null + * character + * + * Counts the number of bytes occupied by a u16 string + * + * @in: null terminated u16 string + * Return: bytes in a u16 string + * + */ +size_t u16_strsize(const void *in); + /** * u16_strlen - count non-zero words * diff --git a/lib/charset.c b/lib/charset.c index 1c6a7f693d..a28034ee1f 100644 --- a/lib/charset.c +++ b/lib/charset.c @@ -379,6 +379,11 @@ size_t u16_strnlen(const u16 *in, size_t count) return i; } +size_t u16_strsize(const void *in) +{ + return (u16_strlen(in) + 1) * sizeof(u16); +} + u16 *u16_strcpy(u16 *dest, const u16 *src) { u16 *tmp = dest; From efe3b5c8dfe8eef46fe951146b8e23fab04e040a Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 9 May 2020 09:16:49 +0200 Subject: [PATCH 07/10] test: unit test for u16_strsize() Provide a test for new Unicode library function u16_strsize(). Signed-off-by: Heinrich Schuchardt --- test/unicode_ut.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/unicode_ut.c b/test/unicode_ut.c index 4d99c20bc0..d8b4aa9b44 100644 --- a/test/unicode_ut.c +++ b/test/unicode_ut.c @@ -581,6 +581,16 @@ static int unicode_test_u16_strncmp(struct unit_test_state *uts) } UNICODE_TEST(unicode_test_u16_strncmp); +static int unicode_test_u16_strsize(struct unit_test_state *uts) +{ + ut_asserteq_64(u16_strsize(c1), 14); + ut_asserteq_64(u16_strsize(c2), 18); + ut_asserteq_64(u16_strsize(c3), 8); + ut_asserteq_64(u16_strsize(c4), 14); + return 0; +} +UNICODE_TEST(unicode_test_u16_strsize); + int do_ut_unicode(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { struct unit_test *tests = ll_entry_start(struct unit_test, unicode_test); From 311da04a67de3b3ad7e0b168b38c06756c59dca4 Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 9 May 2020 08:57:42 +0200 Subject: [PATCH 08/10] lib: charset: correct function descriptions Change function descriptions to match kernel doc style. Signed-off-by: Heinrich Schuchardt --- include/charset.h | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/include/charset.h b/include/charset.h index c118cbeb9a..5564f3bce5 100644 --- a/include/charset.h +++ b/include/charset.h @@ -55,7 +55,7 @@ size_t utf8_utf16_strnlen(const char *src, size_t count); /** * utf8_utf16_strlen() - length of a utf-8 string after conversion to utf-16 * - * @src: utf-8 string + * @a: utf-8 string * Return: length in u16 after conversion to utf-16 without the * trailing \0. If an invalid UTF-8 sequence is hit one * u16 will be reserved for a replacement character. @@ -75,8 +75,8 @@ int utf8_utf16_strncpy(u16 **dst, const char *src, size_t count); /** * utf8_utf16_strcpy() - copy utf-8 string to utf-16 string * - * @dst: destination buffer - * @src: source buffer + * @d: destination buffer + * @s: source buffer * Return: -1 if the input parameters are invalid */ #define utf8_utf16_strcpy(d, s) utf8_utf16_strncpy((d), (s), SIZE_MAX) @@ -126,7 +126,7 @@ size_t utf16_utf8_strnlen(const u16 *src, size_t count); /** * utf16_utf8_strlen() - length of a utf-16 string after conversion to utf-8 * - * @src: utf-16 string + * @a: utf-16 string * Return: length in bytes after conversion to utf-8 without the * trailing \0. If an invalid UTF-16 sequence is hit one * byte will be reserved for a replacement character. @@ -146,8 +146,8 @@ int utf16_utf8_strncpy(char **dst, const u16 *src, size_t count); /** * utf16_utf8_strcpy() - copy utf-16 string to utf-8 string * - * @dst: destination buffer - * @src: source buffer + * @d: destination buffer + * @s: source buffer * Return: -1 if the input parameters are invalid */ #define utf16_utf8_strcpy(d, s) utf16_utf8_strncpy((d), (s), SIZE_MAX) @@ -168,7 +168,7 @@ s32 utf_to_lower(const s32 code); */ s32 utf_to_upper(const s32 code); -/* +/** * u16_strncmp() - compare two u16 string * * @s1: first string to compare @@ -181,6 +181,18 @@ s32 utf_to_upper(const s32 code); * corresponding u16 in s2 */ int u16_strncmp(const u16 *s1, const u16 *s2, size_t n); + +/** + * u16_strcmp() - compare two u16 string + * + * @s1: first string to compare + * @s2: second string to compare + * Return: 0 if the first n u16 are the same in s1 and s2 + * < 0 if the first different u16 in s1 is less than the + * corresponding u16 in s2 + * > 0 if the first different u16 in s1 is greater than the + * corresponding u16 in s2 + */ #define u16_strcmp(s1, s2) u16_strncmp((s1), (s2), SIZE_MAX) /** @@ -190,7 +202,7 @@ int u16_strncmp(const u16 *s1, const u16 *s2, size_t n); * In the EFI context we explicitly need a function handling u16 strings. * * @in: null terminated u16 string - * ReturnValue: number of non-zero words. + * Return: number of non-zero words. * This is not the number of utf-16 letters! */ size_t u16_strlen(const void *in); @@ -203,7 +215,6 @@ size_t u16_strlen(const void *in); * * @in: null terminated u16 string * Return: bytes in a u16 string - * */ size_t u16_strsize(const void *in); @@ -215,7 +226,7 @@ size_t u16_strsize(const void *in); * * @in: null terminated u16 string * @count: maximum number of words to count - * ReturnValue: number of non-zero words. + * Return: number of non-zero words. * This is not the number of utf-16 letters! */ size_t u16_strnlen(const u16 *in, size_t count); @@ -252,10 +263,10 @@ u16 *u16_strdup(const void *src); * NOTE that a single utf16 character can generate up to 3 utf8 * characters. See MAX_UTF8_PER_UTF16. * - * @dest the destination buffer to write the utf8 characters - * @src the source utf16 string - * @size the number of utf16 characters to convert - * @return the pointer to the first unwritten byte in 'dest' + * @dest: the destination buffer to write the utf8 characters + * @src: the source utf16 string + * @size: the number of utf16 characters to convert + * Return: the pointer to the first unwritten byte in 'dest' */ uint8_t *utf16_to_utf8(uint8_t *dest, const uint16_t *src, size_t size); From de699187b590dbc65b843dafc5b8cbea48d5b97a Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 9 May 2020 09:00:59 +0200 Subject: [PATCH 09/10] doc: add Unicode functions to API description Add include/charset.h to generated HTML documentation Signed-off-by: Heinrich Schuchardt --- doc/api/index.rst | 1 + doc/api/unicode.rst | 7 +++++++ 2 files changed, 8 insertions(+) create mode 100644 doc/api/unicode.rst diff --git a/doc/api/index.rst b/doc/api/index.rst index d484c066c5..2578abd40e 100644 --- a/doc/api/index.rst +++ b/doc/api/index.rst @@ -9,3 +9,4 @@ U-Boot API documentation efi linker_lists serial + unicode diff --git a/doc/api/unicode.rst b/doc/api/unicode.rst new file mode 100644 index 0000000000..3fb6745f84 --- /dev/null +++ b/doc/api/unicode.rst @@ -0,0 +1,7 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +Unicode support +=============== + +.. kernel-doc:: include/charset.h + :internal: From bdb15776f6d93a1fe7902346db06a2a9fd43381e Mon Sep 17 00:00:00 2001 From: Heinrich Schuchardt Date: Sat, 9 May 2020 17:59:19 +0200 Subject: [PATCH 10/10] cmd: efidebug: fix -Werror=type-limits warning Compiling with -Wtype-limits yields: cmd/efidebug.c:968:32: error: comparison is always false due to limited range of data type [-Werror=type-limits] 968 | if (*endp != '\0' || bootnext > 0xffff) { | Remove the superfluous check. Fixes: 59df7e7e77e7 ("cmd: add efidebug command") Signed-off-by: Heinrich Schuchardt --- cmd/efidebug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 668e5c4e87..5cc0a41af3 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -970,7 +970,7 @@ static int do_efi_boot_next(cmd_tbl_t *cmdtp, int flag, return CMD_RET_USAGE; bootnext = (u16)simple_strtoul(argv[1], &endp, 16); - if (*endp != '\0' || bootnext > 0xffff) { + if (*endp) { printf("invalid value: %s\n", argv[1]); r = CMD_RET_FAILURE; goto out;