From f98250033854c79b75e75d1522df5fb3f3142328 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Sun, 18 Jun 2023 09:42:22 +0200 Subject: [PATCH 1/3] boot: Move custom device path string creating into its own function --- src/boot/efi/device-path-util.c | 59 +++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/src/boot/efi/device-path-util.c b/src/boot/efi/device-path-util.c index fe5e3a83ce..203bdd5e65 100644 --- a/src/boot/efi/device-path-util.c +++ b/src/boot/efi/device-path-util.c @@ -38,6 +38,37 @@ EFI_STATUS make_file_device_path(EFI_HANDLE device, const char16_t *file, EFI_DE return EFI_SUCCESS; } +static char16_t *device_path_to_str_internal(const EFI_DEVICE_PATH *dp) { + _cleanup_free_ char16_t *str = NULL; + size_t size = 0; + + for (const EFI_DEVICE_PATH *node = dp; !device_path_is_end(node); + node = device_path_next_node(node)) { + + if (node->Type != MEDIA_DEVICE_PATH || node->SubType != MEDIA_FILEPATH_DP) + return NULL; + + size_t path_size = node->Length; + if (path_size <= offsetof(FILEPATH_DEVICE_PATH, PathName) || path_size % sizeof(char16_t)) + return NULL; + path_size -= offsetof(FILEPATH_DEVICE_PATH, PathName); + + _cleanup_free_ char16_t *old = str; + str = xmalloc(size + path_size); + if (old) { + memcpy(str, old, size); + str[size / sizeof(char16_t) - 1] = '\\'; + } + + memcpy(str + (size / sizeof(char16_t)), + ((uint8_t *) node) + offsetof(FILEPATH_DEVICE_PATH, PathName), + path_size); + size += path_size; + } + + return TAKE_PTR(str); +} + EFI_STATUS device_path_to_str(const EFI_DEVICE_PATH *dp, char16_t **ret) { EFI_DEVICE_PATH_TO_TEXT_PROTOCOL *dp_to_text; EFI_STATUS err; @@ -50,31 +81,9 @@ EFI_STATUS device_path_to_str(const EFI_DEVICE_PATH *dp, char16_t **ret) { if (err != EFI_SUCCESS) { /* If the device path to text protocol is not available we can still do a best-effort attempt * to convert it ourselves if we are given filepath-only device path. */ - - size_t size = 0; - for (const EFI_DEVICE_PATH *node = dp; !device_path_is_end(node); - node = device_path_next_node(node)) { - - if (node->Type != MEDIA_DEVICE_PATH || node->SubType != MEDIA_FILEPATH_DP) - return err; - - size_t path_size = node->Length; - if (path_size <= offsetof(FILEPATH_DEVICE_PATH, PathName) || path_size % sizeof(char16_t)) - return EFI_INVALID_PARAMETER; - path_size -= offsetof(FILEPATH_DEVICE_PATH, PathName); - - _cleanup_free_ char16_t *old = str; - str = xmalloc(size + path_size); - if (old) { - memcpy(str, old, size); - str[size / sizeof(char16_t) - 1] = '\\'; - } - - memcpy(str + (size / sizeof(char16_t)), - ((uint8_t *) node) + offsetof(FILEPATH_DEVICE_PATH, PathName), - path_size); - size += path_size; - } + str = device_path_to_str_internal(dp); + if (!str) + return err; *ret = TAKE_PTR(str); return EFI_SUCCESS; From e53e5c0ac16adf2e29d31fda73656391efc38600 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Sun, 18 Jun 2023 10:44:39 +0200 Subject: [PATCH 2/3] boot: Split log_hexdump() --- src/boot/efi/efi-string.c | 17 +++++++++++++++++ src/boot/efi/efi-string.h | 2 ++ src/boot/efi/log.c | 10 ++++++++++ src/boot/efi/log.h | 4 ++++ src/boot/efi/test-efi-string.c | 20 ++++++++++++++++++++ src/boot/efi/util.c | 24 ------------------------ src/boot/efi/util.h | 4 ---- 7 files changed, 53 insertions(+), 28 deletions(-) diff --git a/src/boot/efi/efi-string.c b/src/boot/efi/efi-string.c index d199410881..6c6d644c51 100644 --- a/src/boot/efi/efi-string.c +++ b/src/boot/efi/efi-string.c @@ -391,6 +391,23 @@ bool efi_fnmatch(const char16_t *pattern, const char16_t *haystack) { DEFINE_PARSE_NUMBER(char, parse_number8); DEFINE_PARSE_NUMBER(char16_t, parse_number16); +char16_t *hexdump(const void *data, size_t size) { + static const char hex[16] = "0123456789abcdef"; + const uint8_t *d = data; + + assert(data || size == 0); + + char16_t *buf = xnew(char16_t, size * 2 + 1); + + for (size_t i = 0; i < size; i++) { + buf[i * 2] = hex[d[i] >> 4]; + buf[i * 2 + 1] = hex[d[i] & 0x0F]; + } + + buf[size * 2] = 0; + return buf; +} + static const char * const warn_table[] = { [EFI_SUCCESS] = "Success", [EFI_WARN_UNKNOWN_GLYPH] = "Unknown glyph", diff --git a/src/boot/efi/efi-string.h b/src/boot/efi/efi-string.h index b97e16990a..9b82bfeca0 100644 --- a/src/boot/efi/efi-string.h +++ b/src/boot/efi/efi-string.h @@ -108,6 +108,8 @@ bool efi_fnmatch(const char16_t *pattern, const char16_t *haystack); bool parse_number8(const char *s, uint64_t *ret_u, const char **ret_tail); bool parse_number16(const char16_t *s, uint64_t *ret_u, const char16_t **ret_tail); +char16_t *hexdump(const void *data, size_t size); + #ifdef __clang__ # define _gnu_printf_(a, b) _printf_(a, b) #else diff --git a/src/boot/efi/log.c b/src/boot/efi/log.c index 6d0edec2cf..93631aca94 100644 --- a/src/boot/efi/log.c +++ b/src/boot/efi/log.c @@ -3,6 +3,7 @@ #include "log.h" #include "proto/rng.h" #include "proto/simple-text-io.h" +#include "util.h" static unsigned log_count = 0; @@ -52,6 +53,15 @@ EFI_STATUS log_internal(EFI_STATUS status, const char *format, ...) { return status; } +#ifdef EFI_DEBUG +void log_hexdump(const char16_t *prefix, const void *data, size_t size) { + /* Debugging helper — please keep this around, even if not used */ + + _cleanup_free_ char16_t *hex = hexdump(data, size); + log_internal(EFI_SUCCESS, "%ls[%zu]: %ls", prefix, size, hex); +} +#endif + void log_wait(void) { if (log_count == 0) return; diff --git a/src/boot/efi/log.h b/src/boot/efi/log.h index 973e13c260..13f3887b8a 100644 --- a/src/boot/efi/log.h +++ b/src/boot/efi/log.h @@ -26,3 +26,7 @@ _gnu_printf_(2, 3) EFI_STATUS log_internal(EFI_STATUS status, const char *format #define log_error(...) log_internal(EFI_INVALID_PARAMETER, __VA_ARGS__) #define log_oom() log_internal(EFI_OUT_OF_RESOURCES, "Out of memory.") #define log_trace() log_internal(EFI_SUCCESS, "%s:%i@%s", __FILE__, __LINE__, __func__) + +#ifdef EFI_DEBUG +void log_hexdump(const char16_t *prefix, const void *data, size_t size); +#endif diff --git a/src/boot/efi/test-efi-string.c b/src/boot/efi/test-efi-string.c index d214b1536e..8f777c1ce3 100644 --- a/src/boot/efi/test-efi-string.c +++ b/src/boot/efi/test-efi-string.c @@ -484,6 +484,26 @@ TEST(parse_number16) { assert_se(streq16(tail, u"rest")); } +TEST(test_hexdump) { + char16_t *hex; + + hex = hexdump(NULL, 0); + assert(streq16(hex, u"")); + free(hex); + + hex = hexdump("1", 2); + assert(streq16(hex, u"3100")); + free(hex); + + hex = hexdump("abc", 4); + assert(streq16(hex, u"61626300")); + free(hex); + + hex = hexdump((uint8_t[]){ 0x0, 0x42, 0xFF, 0xF1, 0x1F }, 5); + assert(streq16(hex, u"0042fff11f")); + free(hex); +} + _printf_(1, 2) static void test_printf_one(const char *format, ...) { va_list ap, ap_efi; va_start(ap, format); diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index 42526d7005..d2b8f88250 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -568,30 +568,6 @@ __attribute__((noinline)) void notify_debugger(const char *identity, volatile bo #endif } -#ifdef EFI_DEBUG -void hexdump(const char16_t *prefix, const void *data, size_t size) { - static const char hex[16] = "0123456789abcdef"; - _cleanup_free_ char16_t *buf = NULL; - const uint8_t *d = data; - - assert(prefix); - assert(data || size == 0); - - /* Debugging helper — please keep this around, even if not used */ - - buf = xnew(char16_t, size*2+1); - - for (size_t i = 0; i < size; i++) { - buf[i*2] = hex[d[i] >> 4]; - buf[i*2+1] = hex[d[i] & 0x0F]; - } - - buf[size*2] = 0; - - log_error("%ls[%zu]: %ls", prefix, size, buf); -} -#endif - #if defined(__i386__) || defined(__x86_64__) static inline uint8_t inb(uint16_t port) { uint8_t value; diff --git a/src/boot/efi/util.h b/src/boot/efi/util.h index aadbbdad0d..03cd928b2f 100644 --- a/src/boot/efi/util.h +++ b/src/boot/efi/util.h @@ -167,10 +167,6 @@ uint64_t get_os_indications_supported(void); * be attached. See debug-sd-boot.sh for how this can be done. */ void notify_debugger(const char *identity, bool wait); -#ifdef EFI_DEBUG -void hexdump(const char16_t *prefix, const void *data, size_t size); -#endif - /* On x86 the compiler assumes a different incoming stack alignment than what we get. * This will cause long long variables to be misaligned when building with * '-mlong-double' (for correct struct layouts). Normally, the compiler realigns the From 78fffdea37fc9456b76b21dc94e8cb9628986f65 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Sun, 18 Jun 2023 10:54:20 +0200 Subject: [PATCH 3/3] boot: Improve device_path_to_str_internal() The UEFI spec has a generic `Path` node representation that can be used for device path nodes that are unknown. So we can use that instead of giving up when we see a node other than FilePath. This also simplifies the FilePath case by just using xasprintf(). The code is really just a fallback for silly firmware that does not implement EFI_DEVICE_PATH_TO_TEXT_PROTOCOL (looking at you, Apple). The correctness of this was tested by round-tripping it through EFI_DEVICE_PATH_FROM_TEXT_PROTOCOL, which yielded an identical device compared to our input path. --- src/boot/efi/device-path-util.c | 59 +++++++++++++++++---------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/src/boot/efi/device-path-util.c b/src/boot/efi/device-path-util.c index 203bdd5e65..2a85e8bbfc 100644 --- a/src/boot/efi/device-path-util.c +++ b/src/boot/efi/device-path-util.c @@ -39,34 +39,41 @@ EFI_STATUS make_file_device_path(EFI_HANDLE device, const char16_t *file, EFI_DE } static char16_t *device_path_to_str_internal(const EFI_DEVICE_PATH *dp) { - _cleanup_free_ char16_t *str = NULL; - size_t size = 0; - - for (const EFI_DEVICE_PATH *node = dp; !device_path_is_end(node); - node = device_path_next_node(node)) { - - if (node->Type != MEDIA_DEVICE_PATH || node->SubType != MEDIA_FILEPATH_DP) - return NULL; - - size_t path_size = node->Length; - if (path_size <= offsetof(FILEPATH_DEVICE_PATH, PathName) || path_size % sizeof(char16_t)) - return NULL; - path_size -= offsetof(FILEPATH_DEVICE_PATH, PathName); + char16_t *str = NULL; + for (const EFI_DEVICE_PATH *node = dp; !device_path_is_end(node); node = device_path_next_node(node)) { _cleanup_free_ char16_t *old = str; - str = xmalloc(size + path_size); - if (old) { - memcpy(str, old, size); - str[size / sizeof(char16_t) - 1] = '\\'; + + if (node->Type == END_DEVICE_PATH_TYPE && node->SubType == END_INSTANCE_DEVICE_PATH_SUBTYPE) { + str = xasprintf("%ls%s,", strempty(old), old ? "\\" : ""); + continue; } - memcpy(str + (size / sizeof(char16_t)), - ((uint8_t *) node) + offsetof(FILEPATH_DEVICE_PATH, PathName), - path_size); - size += path_size; + /* Special-case this so that FilePath-only device path string look and behave nicely. */ + if (node->Type == MEDIA_DEVICE_PATH && node->SubType == MEDIA_FILEPATH_DP) { + str = xasprintf("%ls%s%ls", + strempty(old), + old ? "\\" : "", + ((FILEPATH_DEVICE_PATH *) node)->PathName); + continue; + } + + /* Instead of coding all the different types and sub-types here we just use the + * generic node form. This function is a best-effort for firmware that does not + * provide the EFI_DEVICE_PATH_TO_TEXT_PROTOCOL after all. */ + + size_t size = node->Length - sizeof(EFI_DEVICE_PATH); + _cleanup_free_ char16_t *hex_data = hexdump((uint8_t *) node + sizeof(EFI_DEVICE_PATH), size); + str = xasprintf("%ls%sPath(%u,%u%s%ls)", + strempty(old), + old ? "/" : "", + node->Type, + node->SubType, + size == 0 ? "" : ",", + hex_data); } - return TAKE_PTR(str); + return str; } EFI_STATUS device_path_to_str(const EFI_DEVICE_PATH *dp, char16_t **ret) { @@ -79,13 +86,7 @@ EFI_STATUS device_path_to_str(const EFI_DEVICE_PATH *dp, char16_t **ret) { err = BS->LocateProtocol(MAKE_GUID_PTR(EFI_DEVICE_PATH_TO_TEXT_PROTOCOL), NULL, (void **) &dp_to_text); if (err != EFI_SUCCESS) { - /* If the device path to text protocol is not available we can still do a best-effort attempt - * to convert it ourselves if we are given filepath-only device path. */ - str = device_path_to_str_internal(dp); - if (!str) - return err; - - *ret = TAKE_PTR(str); + *ret = device_path_to_str_internal(dp); return EFI_SUCCESS; }