Am 18. September 2026 10:51:42 MESZ schrieb "Vincent Stehlé"
<[email protected]>:
>On Thu, Sep 17, 2026 at 06:23:11PM +0200, Heinrich Schuchardt wrote:
>> Am 17. September 2026 11:45:31 MESZ schrieb "Vincent Stehlé"
>> <[email protected]>:
>> >The new_string() and set_string() HII string protocol functions keep
>> >using old pointers after calling realloc(), which may have moved.
>> >
>> >The set_string() function also assumes that reallocation only grew the
>> >array by one element when computing the start of the memory to clear,
>> >and forgets to update nstrings to the new table size after reallocation.
>> >
>> >Repair and factorize the reallocation code in a new grow_string_table()
>> >function to fix those issues.
>>
>>
>> Should we add a test in U-Boot and possibly in SCT?
>
>Hi Heinrich,
>
>Thanks for having a look.
>
>I think the existing "HII database protocols" self-test is sufficient, thanks
>to
>the change.
>
>With this test, before the patch, the code coverage is fairly good in
>new_string() but is poor in set_string().
>
>After the patch, the code coverage of grow_string_table() is fairly good as
>expected, and the poor code coverage of set_string() is "gone".
>
>Best regards,
>Vincent.
If the test did not fail before your code was added, there must be some test
case missing.
Best regards
Heinrich
>
>>
>> Best regards
>>
>> Heinrich
>>
>> >
>> >Fixes: c9bfb22296b0 ("efi_loader: Initial HII database protocols")
>> >Signed-off-by: Vincent Stehlé <[email protected]>
>> >Cc: Heinrich Schuchardt <[email protected]>
>> >Cc: Ilias Apalodimas <[email protected]>
>> >Cc: Tom Rini <[email protected]>
>> >---
>> >This can be tested on the sandbox with the following command:
>> >
>> > ./u-boot -T -c 'setenv efi_selftest HII database protocols; bootefi
>> > selftest'
>> >
>> >While the unit test does cause reallocation with new_string(), it does not
>> >with set_string().
>> >
>> >Further testing can be done by forcing stbl->strings = NULL, just after
>> >calling realloc(stbl->strings, ...).
>> >---
>> > lib/efi_loader/efi_hii.c | 64
>> > +++++++++++++++++++++++++++---------------------
>> > 1 file changed, 36 insertions(+), 28 deletions(-)
>> >
>> >diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
>> >index 7bf51ad43d1..90dff91e5d4 100644
>> >--- a/lib/efi_loader/efi_hii.c
>> >+++ b/lib/efi_loader/efi_hii.c
>> >@@ -822,6 +822,37 @@ static bool language_match(char *language, char
>> >*languages)
>> > return false;
>> > }
>> >
>> >+/**
>> >+ * grow_string_table() - Grow a string table if necessary.
>> >+ * @stbl: String table to grow.
>> >+ * @n: Minimum table size desired.
>> >+ *
>> >+ * Make sure a string table can contain at least n elements.
>> >+ * If the table is too small, reallocate the memory and clear the new
>> >elements.
>> >+ *
>> >+ * Return: false in case of reallocation error, true otherwise.
>> >+ * In case of error, the original string table is left untouched.
>> >+ * In case of success, stbl->strings and stbl->nstrings may have been
>> >updated.
>> >+ */
>> >+static bool grow_string_table(struct efi_string_table *stbl, u32 n)
>> >+{
>> >+ void *buf;
>> >+ u32 old_n = stbl->nstrings;
>> >+
>> >+ if (old_n >= n)
>> >+ return true;
>> >+
>> >+ buf = realloc(stbl->strings, sizeof(stbl->strings[0]) * n);
>> >+ if (!buf)
>> >+ return false;
>> >+
>> >+ stbl->strings = buf;
>> >+ stbl->nstrings = n;
>> >+ memset(&stbl->strings[old_n], 0, sizeof(stbl->strings[0]) * (n -
>> >old_n));
>> >+
>> >+ return true;
>> >+}
>> >+
>> > static efi_status_t EFIAPI
>> > new_string(const struct efi_hii_string_protocol *this,
>> > efi_hii_handle_t package_list,
>> >@@ -847,23 +878,12 @@ new_string(const struct efi_hii_string_protocol *this,
>> > list_for_each_entry(stbl, &hii->string_tables, link) {
>> > if (language_match((char *)language, stbl->language)) {
>> > efi_string_id_t new_id;
>> >- void *buf;
>> > efi_string_t str;
>> >
>> > new_id = ++hii->max_string_id;
>> >- if (stbl->nstrings < new_id) {
>> >- buf = realloc(stbl->strings,
>> >- sizeof(stbl->strings[0])
>> >- * new_id);
>> >- if (!buf)
>> >- return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> >-
>> >- memset(&stbl->strings[stbl->nstrings], 0,
>> >- (new_id - stbl->nstrings)
>> >- * sizeof(stbl->strings[0]));
>> >- stbl->strings = buf;
>> >- stbl->nstrings = new_id;
>> >- }
>> >+
>> >+ if (!grow_string_table(stbl, new_id))
>> >+ return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> >
>> > str = u16_strdup(string);
>> > if (!str)
>> >@@ -957,20 +977,8 @@ set_string(const struct efi_hii_string_protocol *this,
>> > if (hii->max_string_id < string_id)
>> > return EFI_EXIT(EFI_NOT_FOUND);
>> >
>> >- if (stbl->nstrings < string_id) {
>> >- void *buf;
>> >-
>> >- buf = realloc(stbl->strings,
>> >- string_id
>> >- * sizeof(stbl->strings[0]));
>> >- if (!buf)
>> >- return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> >-
>> >- memset(&stbl->strings[string_id - 1], 0,
>> >- (string_id - stbl->nstrings)
>> >- * sizeof(stbl->strings[0]));
>> >- stbl->strings = buf;
>> >- }
>> >+ if (!grow_string_table(stbl, string_id))
>> >+ return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> >
>> > str = u16_strdup(string);
>> > if (!str)
>> >
>> >---
>> >base-commit: 2c7eaee7b3422ce7c84e892abfe16e1afce42e83
>> >change-id: 20260916-ptr-670be74dcbd3
>> >
>> >Best regards,
>> >--
>> >Vincent.
>> >
>>
>
