Re: [PATCH] efi_loader: fix string table reallocations

["Vincent Stehlé" <vincent.stehle@arm.com>]

On Fri, Sep 18, 2026 at 06:22:23PM +0200, Heinrich Schuchardt wrote:
> Am 18. September 2026 10:51:42 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
> >On Thu, Sep 17, 2026 at 06:23:11PM +0200, Heinrich Schuchardt wrote:
> >> Am 17. September 2026 11:45:31 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
> >> >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.

Hi Heinrich,

Yes, we are indeed missing a test, which would exercise set_string() in
a way that would reallocate the string table, and I did not find an easy
way to obtain that with the existing HII packages data that we have
currently (packagelist1 & 2).

Arguably, now that the factorized function is well covered with
new_string(), we might not even have to do an additional testing effort,
but if you insist I could try to add some more packages data and a
specific unit test.
Just let me know if you prefer that.

Best regards,
Vincent.

> 
> Best regards
> 
> Heinrich
> 
> 
> >
> >> 
> >> Best regards
> >> 
> >> Heinrich
> >> 
> >> >
> >> >Fixes: c9bfb22296b0 ("efi_loader: Initial HII database protocols")
> >> >Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> >> >Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> >Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >> >Cc: Tom Rini <trini@konsulko.com>
> >> >---
> >> >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.
> >> >
> >> 
> >
> 

-- 
Vincent.