On 10/23/26 14:16, Ilias Apalodimas wrote:
)
On Fri, 23 Oct 2026 at 14:57, Heinrich Schuchardt
<[email protected]> wrote:
On 10/23/26 13:39, Ilias Apalodimas wrote:
On Fri, 23 Oct 2026 at 14:19, Heinrich Schuchardt
<[email protected]> wrote:
On 10/23/26 09:25, Ilias Apalodimas wrote:
Hi Heinrich
On Fri, 23 Oct 2026 at 02:56, Heinrich Schuchardt
<[email protected]> wrote:
* Add missing parameter checks required by the UEFI spec.
* Return EFI_INVALID_LANGUAGE if a string does not exist in the requested
language.
* Handle querying the string size with *string_size == 0 and string = NULL.
Fixes: c9bfb22296b0 ("efi_loader: Initial HII database protocols")
Signed-off-by: Heinrich Schuchardt <[email protected]>
---
v3:
Update commit message to indicate that parameter checks are added.
Correct a misspelled variable.
v2:
string_id == 0 is not used by our code. Return EFI_NOT_FOUND.
---
lib/efi_loader/efi_hii.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
index 90dff91e5d4..47325f7d8aa 100644
--- a/lib/efi_loader/efi_hii.c
+++ b/lib/efi_loader/efi_hii.c
@@ -910,41 +910,55 @@ get_string(const struct efi_hii_string_protocol *this,
{
struct efi_hii_packagelist *hii = package_list;
struct efi_string_table *stbl;
+ efi_status_t ret;
EFI_ENTRY("%p, \"%s\", %p, %u, %p, %p, %p", this, language,
package_list, string_id, string, string_size,
string_font_info);
- if (!package_list || !efi_hii_packagelist_exists(package_list))
- return EFI_EXIT(EFI_NOT_FOUND);
+ if (!language || !string_size || (*string_size && !string)) {
+ ret = EFI_INVALID_PARAMETER;
+ goto out;
+ }
+
+ if (!package_list ||
+ !efi_hii_packagelist_exists(package_list) ||
+ !string_id ||
+ string_id > hii->max_string_id) {
+ ret = EFI_NOT_FOUND;
+ goto out;
+ }
+ ret = EFI_INVALID_LANGUAGE;
list_for_each_entry(stbl, &hii->string_tables, link) {
if (language_match((char *)language, stbl->language)) {
efi_string_t str;
size_t len;
if (stbl->nstrings < string_id)
- return EFI_EXIT(EFI_NOT_FOUND);
+ goto out;
str = stbl->strings[string_id - 1].string;
if (str) {
len = u16_strsize(str);
if (*string_size < len) {
*string_size = len;
-
- return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
+ ret = EFI_BUFFER_TOO_SMALL;
+ goto out;
}
- memcpy(string, str, len);
*string_size = len;
+ if (string)
This is always going to be true since u16_strsize() include the
terminator. It's a nit so I am fine fixing it during the merge
Reviewed-by: Ilias Apalodimas <[email protected]>
string is the pointer variable passed into GetString().
The value of string is not changed by our implementation.
The UEFI specification allows to call the GetString() service with
String == NULL && StringLength == 0
to retrieve the length of the buffer before allocating a buffer for it.
We must not call memcpy() if string is NULL.
I am not sure I follow you here. If string is NULL, aren't we exiting
really early at the beginning of the function?
Thanks
/Ilias
For String == NULL && *StringLength == 0 (I forgot the * above) the term
(*string_size && !string)
is not true and we don't quit with EFI_INVALID_PARAMETER. Instead we
reach the loop where we look for a string matching the string ID.
But in the case you are describing
len = u16_strsize(str); <---- len is 2
The next if check will succeed since *string_size is 0 and we will
exit with EFI_BUFFER_TOO_SMALL.
You are right that check needs to move down.
Best regards
Heinrich
Thanks
/Ilias
We even have a test for this in
lib/efi_selftest/efi_selftest_hii.c:766ff.
Best regards
Heinrich
+ memcpy(string, str, len);
+ ret = EFI_SUCCESS;
+ goto out;
} else {
- return EFI_EXIT(EFI_NOT_FOUND);
+ goto out;
}
-
- return EFI_EXIT(EFI_SUCCESS);
}
}
- return EFI_EXIT(EFI_NOT_FOUND);
+out:
+ return EFI_EXIT(ret);
}
static efi_status_t EFIAPI
--
2.53.0
