From 72bd3458e53bbdea2d96fd61271dbea2c4aee5bf Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 1 Jun 2022 13:35:29 +0200 Subject: [PATCH 1/3] boot: Add parse_number8/16 --- src/boot/efi/efi-string.c | 32 ++++++++++++++++++++ src/boot/efi/efi-string.h | 3 ++ src/boot/efi/test-efi-string.c | 54 ++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+) diff --git a/src/boot/efi/efi-string.c b/src/boot/efi/efi-string.c index b9ef1548ca..b1d83cb483 100644 --- a/src/boot/efi/efi-string.c +++ b/src/boot/efi/efi-string.c @@ -239,6 +239,38 @@ bool efi_fnmatch(const char16_t *pattern, const char16_t *haystack) { return efi_fnmatch_internal(pattern, haystack, 32); } +#define DEFINE_PARSE_NUMBER(type, name) \ + bool name(const type *s, uint64_t *ret_u, const type **ret_tail) { \ + assert(ret_u); \ + \ + if (!s) \ + return false; \ + \ + /* Need at least one digit. */ \ + if (*s < '0' || *s > '9') \ + return false; \ + \ + uint64_t u = 0; \ + while (*s >= '0' && *s <= '9') { \ + if (__builtin_mul_overflow(u, 10, &u)) \ + return false; \ + if (__builtin_add_overflow(u, *s - '0', &u)) \ + return false; \ + s++; \ + } \ + \ + if (!ret_tail && *s != '\0') \ + return false; \ + \ + *ret_u = u; \ + if (ret_tail) \ + *ret_tail = s; \ + return true; \ + } + +DEFINE_PARSE_NUMBER(char, parse_number8); +DEFINE_PARSE_NUMBER(char16_t, parse_number16); + int efi_memcmp(const void *p1, const void *p2, size_t n) { const uint8_t *up1 = p1, *up2 = p2; int r; diff --git a/src/boot/efi/efi-string.h b/src/boot/efi/efi-string.h index 55c9c6e47a..c25349c0e1 100644 --- a/src/boot/efi/efi-string.h +++ b/src/boot/efi/efi-string.h @@ -101,6 +101,9 @@ static inline char16_t *xstrdup16(const char16_t *s) { 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); + #ifdef SD_BOOT /* The compiler normally has knowledge about standard functions such as memcmp, but this is not the case when * compiling with -ffreestanding. By referring to builtins, the compiler can check arguments and do diff --git a/src/boot/efi/test-efi-string.c b/src/boot/efi/test-efi-string.c index 178ad766cb..0bfb564e3d 100644 --- a/src/boot/efi/test-efi-string.c +++ b/src/boot/efi/test-efi-string.c @@ -371,6 +371,60 @@ TEST(efi_fnmatch) { TEST_FNMATCH_ONE("?a*b[.-0]c", "/a/b/c", true); } +TEST(parse_number8) { + uint64_t u; + const char *tail; + + assert_se(!parse_number8(NULL, &u, NULL)); + assert_se(!parse_number8("", &u, NULL)); + assert_se(!parse_number8("a1", &u, NULL)); + assert_se(!parse_number8("1a", &u, NULL)); + assert_se(!parse_number8("-42", &u, NULL)); + assert_se(!parse_number8("18446744073709551616", &u, NULL)); + + assert_se(parse_number8("0", &u, NULL)); + assert_se(u == 0); + assert_se(parse_number8("1", &u, NULL)); + assert_se(u == 1); + assert_se(parse_number8("999", &u, NULL)); + assert_se(u == 999); + assert_se(parse_number8("18446744073709551615", &u, NULL)); + assert_se(u == UINT64_MAX); + assert_se(parse_number8("42", &u, &tail)); + assert_se(u == 42); + assert_se(streq8(tail, "")); + assert_se(parse_number8("54321rest", &u, &tail)); + assert_se(u == 54321); + assert_se(streq8(tail, "rest")); +} + +TEST(parse_number16) { + uint64_t u; + const char16_t *tail; + + assert_se(!parse_number16(NULL, &u, NULL)); + assert_se(!parse_number16(u"", &u, NULL)); + assert_se(!parse_number16(u"a1", &u, NULL)); + assert_se(!parse_number16(u"1a", &u, NULL)); + assert_se(!parse_number16(u"-42", &u, NULL)); + assert_se(!parse_number16(u"18446744073709551616", &u, NULL)); + + assert_se(parse_number16(u"0", &u, NULL)); + assert_se(u == 0); + assert_se(parse_number16(u"1", &u, NULL)); + assert_se(u == 1); + assert_se(parse_number16(u"999", &u, NULL)); + assert_se(u == 999); + assert_se(parse_number16(u"18446744073709551615", &u, NULL)); + assert_se(u == UINT64_MAX); + assert_se(parse_number16(u"42", &u, &tail)); + assert_se(u == 42); + assert_se(streq16(tail, u"")); + assert_se(parse_number16(u"54321rest", &u, &tail)); + assert_se(u == 54321); + assert_se(streq16(tail, u"rest")); +} + TEST(efi_memcmp) { assert_se(efi_memcmp(NULL, NULL, 0) == 0); assert_se(efi_memcmp(NULL, NULL, 1) == 0); From 1621ab4600e5b7cdcd20e00257eb720683097a50 Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Wed, 1 Jun 2022 13:42:23 +0200 Subject: [PATCH 2/3] boot: Drop use of Atoi --- src/boot/efi/boot.c | 20 ++++++++++++-------- src/boot/efi/util.c | 11 ++++++++--- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 3833ae9461..7d881fc752 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -1159,10 +1159,12 @@ static void config_defaults_load_from_file(Config *config, CHAR8 *content) { else if (streq8((char *) value, "menu-hidden")) config->timeout_sec_config = TIMEOUT_MENU_HIDDEN; else { - _cleanup_freepool_ CHAR16 *s = NULL; - - s = xstra_to_str(value); - config->timeout_sec_config = MIN(Atoi(s), TIMEOUT_TYPE_MAX); + uint64_t u; + if (!parse_number8((char *) value, &u, NULL) || u > TIMEOUT_TYPE_MAX) { + log_error_stall(L"Error parsing 'timeout' config option: %a", value); + continue; + } + config->timeout_sec_config = u; } config->timeout_sec = config->timeout_sec_config; continue; @@ -1221,10 +1223,12 @@ static void config_defaults_load_from_file(Config *config, CHAR8 *content) { else if (streq8((char *) value, "keep")) config->console_mode = CONSOLE_MODE_KEEP; else { - _cleanup_freepool_ CHAR16 *s = NULL; - - s = xstra_to_str(value); - config->console_mode = MIN(Atoi(s), (UINTN)CONSOLE_MODE_RANGE_MAX); + uint64_t u; + if (!parse_number8((char *) value, &u, NULL) || u > CONSOLE_MODE_RANGE_MAX) { + log_error_stall(L"Error parsing 'console-mode' config option: %a", value); + continue; + } + config->console_mode = u; } continue; } diff --git a/src/boot/efi/util.c b/src/boot/efi/util.c index c6324035ad..8ec7e17efc 100644 --- a/src/boot/efi/util.c +++ b/src/boot/efi/util.c @@ -127,16 +127,21 @@ EFI_STATUS efivar_get(const EFI_GUID *vendor, const CHAR16 *name, CHAR16 **value EFI_STATUS efivar_get_uint_string(const EFI_GUID *vendor, const CHAR16 *name, UINTN *i) { _cleanup_freepool_ CHAR16 *val = NULL; EFI_STATUS err; + uint64_t u; assert(vendor); assert(name); assert(i); err = efivar_get(vendor, name, &val); - if (!EFI_ERROR(err)) - *i = Atoi(val); + if (err != EFI_SUCCESS) + return err; - return err; + if (!parse_number16(val, &u, NULL) || u > UINTN_MAX) + return EFI_INVALID_PARAMETER; + + *i = u; + return EFI_SUCCESS; } EFI_STATUS efivar_get_uint32_le(const EFI_GUID *vendor, const CHAR16 *name, UINT32 *ret) { From 153381952b32885d328cbc3e36f486454be3fcfd Mon Sep 17 00:00:00 2001 From: Jan Janssen Date: Thu, 2 Jun 2022 11:23:12 +0200 Subject: [PATCH 3/3] boot: Use parse_number16 for boot counter parsing --- src/boot/efi/boot.c | 151 ++++++++++++++------------------------------ 1 file changed, 48 insertions(+), 103 deletions(-) diff --git a/src/boot/efi/boot.c b/src/boot/efi/boot.c index 7d881fc752..ea80c1875d 100644 --- a/src/boot/efi/boot.c +++ b/src/boot/efi/boot.c @@ -65,8 +65,8 @@ typedef struct { CHAR16 **initrd; CHAR16 key; EFI_STATUS (*call)(void); - UINTN tries_done; - UINTN tries_left; + int tries_done; + int tries_left; CHAR16 *path; CHAR16 *current_name; CHAR16 *next_name; @@ -567,9 +567,9 @@ static void print_status(Config *config, CHAR16 *loaded_image_path) { ps_string(L" options: %s\n", entry->options); ps_bool(L" internal call: %s\n", !!entry->call); - ps_bool(L"counting boots: %s\n", entry->tries_left != UINTN_MAX); - if (entry->tries_left != UINTN_MAX) { - Print(L" tries: %" PRIuN L" done, %" PRIuN L" left\n", entry->tries_done, entry->tries_left); + ps_bool(L"counting boots: %s\n", entry->tries_left >= 0); + if (entry->tries_left >= 0) { + Print(L" tries: %u left, %u done\n", entry->tries_left, entry->tries_done); Print(L" current path: %s\\%s\n", entry->path, entry->current_name); Print(L" next path: %s\\%s\n", entry->path, entry->next_name); } @@ -1262,12 +1262,10 @@ static void config_entry_parse_tries( const CHAR16 *file, const CHAR16 *suffix) { - UINTN left = UINTN_MAX, done = UINTN_MAX, factor = 1, i, next_left, next_done; - _cleanup_freepool_ CHAR16 *prefix = NULL; - assert(entry); assert(path); assert(file); + assert(suffix); /* * Parses a suffix of two counters (one going down, one going up) in the form "+LEFT-DONE" from the end of the @@ -1280,99 +1278,46 @@ static void config_entry_parse_tries( * foobar+4-0.efi → foobar+3-1.efi → foobar+2-2.efi → foobar+1-3.efi → foobar+0-4.efi → STOP! */ - i = strlen16(file); - - /* Chop off any suffix such as ".conf" or ".efi" */ - if (suffix) { - UINTN suffix_length; - - suffix_length = strlen16(suffix); - if (i < suffix_length) - return; - - i -= suffix_length; - } - - /* Go backwards through the string and parse everything we encounter */ + const char16_t *counter = NULL; for (;;) { - if (i == 0) - return; - - i--; - - switch (file[i]) { - - case '+': - if (left == UINTN_MAX) /* didn't read at least one digit for 'left'? */ - return; - - if (done == UINTN_MAX) /* no 'done' counter? If so, it's equivalent to 0 */ - done = 0; - - goto good; - - case '-': - if (left == UINTN_MAX) /* didn't parse any digit yet? */ - return; - - if (done != UINTN_MAX) /* already encountered a dash earlier? */ - return; - - /* So we encountered a dash. This means this counter is of the form +LEFT-DONE. Let's assign - * what we already parsed to 'done', and start fresh for the 'left' part. */ - - done = left; - left = UINTN_MAX; - factor = 1; + char16_t *plus = strchr16(counter ?: file, '+'); + if (plus) { + /* We want the last "+". */ + counter = plus + 1; + continue; + } + if (counter) break; - case '0'...'9': { - UINTN new_factor; - - if (left == UINTN_MAX) - left = file[i] - '0'; - else { - UINTN new_left, digit; - - digit = file[i] - '0'; - if (digit > UINTN_MAX / factor) /* overflow check */ - return; - - new_left = left + digit * factor; - if (new_left < left) /* overflow check */ - return; - - if (new_left == UINTN_MAX) /* don't allow us to be confused */ - return; - } - - new_factor = factor * 10; - if (new_factor < factor) /* overflow check */ - return; - - factor = new_factor; - break; - } - - default: - return; - } + /* No boot counter found. */ + return; } -good: - entry->tries_left = left; - entry->tries_done = done; + uint64_t tries_left, tries_done = 0; + size_t prefix_len = counter - file; + if (!parse_number16(counter, &tries_left, &counter) || tries_left > INT_MAX) + return; + + /* Parse done counter only if present. */ + if (*counter == '-' && (!parse_number16(counter + 1, &tries_done, &counter) || tries_done > INT_MAX)) + return; + + /* Boot counter in the middle of the name? */ + if (!streq16(counter, suffix)) + return; + + entry->tries_left = tries_left; + entry->tries_done = tries_done; entry->path = xstrdup16(path); entry->current_name = xstrdup16(file); - - next_left = left <= 0 ? 0 : left - 1; - next_done = done >= (UINTN) -2 ? (UINTN) -2 : done + 1; - - prefix = xstrdup16(file); - prefix[i] = 0; - - entry->next_name = xpool_print(L"%s+%" PRIuN L"-%" PRIuN L"%s", prefix, next_left, next_done, strempty(suffix)); + entry->next_name = xpool_print( + L"%.*s%u-%u%s", + prefix_len, + file, + LESS_BY(tries_left, 1u), + MIN(tries_done + 1, (uint64_t) INT_MAX), + suffix); } static void config_entry_bump_counters(ConfigEntry *entry, EFI_FILE *root_dir) { @@ -1385,7 +1330,7 @@ static void config_entry_bump_counters(ConfigEntry *entry, EFI_FILE *root_dir) { assert(entry); assert(root_dir); - if (entry->tries_left == UINTN_MAX) + if (entry->tries_left < 0) return; if (!entry->path || !entry->current_name || !entry->next_name) @@ -1448,8 +1393,8 @@ static void config_entry_add_type1( entry = xnew(ConfigEntry, 1); *entry = (ConfigEntry) { - .tries_done = UINTN_MAX, - .tries_left = UINTN_MAX, + .tries_done = -1, + .tries_left = -1, }; while ((line = line_get_key_value(content, (CHAR8 *)" \t", &pos, &key, &value))) { @@ -1706,7 +1651,7 @@ static int config_entry_compare(const ConfigEntry *a, const ConfigEntry *b) { if (r != 0) return r; - if (a->tries_left == UINTN_MAX || b->tries_left == UINTN_MAX) + if (a->tries_left < 0 || b->tries_left < 0) return 0; /* If both items have boot counting, and otherwise are identical, put the entry with more tries left first */ @@ -1927,8 +1872,8 @@ static ConfigEntry *config_entry_add_loader_auto( .device = device, .loader = xstrdup16(loader), .key = key, - .tries_done = UINTN_MAX, - .tries_left = UINTN_MAX, + .tries_done = -1, + .tries_left = -1, }; config_add_entry(config, entry); @@ -2216,8 +2161,8 @@ static void config_entry_add_unified( .loader = xpool_print(L"\\EFI\\Linux\\%s", f->FileName), .sort_key = xstrdup16(good_sort_key), .key = 'l', - .tries_done = UINTN_MAX, - .tries_left = UINTN_MAX, + .tries_done = -1, + .tries_left = -1, }; strtolower16(entry->id); @@ -2560,8 +2505,8 @@ static void config_load_all_entries( .id = xstrdup16(u"auto-reboot-to-firmware-setup"), .title = xstrdup16(u"Reboot Into Firmware Interface"), .call = reboot_into_firmware, - .tries_done = UINTN_MAX, - .tries_left = UINTN_MAX, + .tries_done = -1, + .tries_left = -1, }; config_add_entry(config, entry); }