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