)
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.
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
> >>>>
> >>
>
