Re: [PATCH v5 6/8] kern/misc, kern/efi: Extract UTF-8 to UTF-16 code

[Daniel Kiper]

On Thu, Mar 30, 2023 at 01:18:11PM +0200, Oliver Steffen wrote:
> Create a new function for UTF-8 to UTF-16 conversion called
> grub_utf8_to_utf16_alloc() in the grub-code/kern/misc.c and replace
> charset conversion code used in some places in the EFI code. It is
> modeled after the grub_utf8_to_ucs4_alloc() like functions in charset.h.
> It can't live in charset.h, because it needs to be reachable from the
> kern/efi code.
>
> Add a check for integer overflow and remove redundant NUL-termination.
>
> Signed-off-by: Oliver Steffen <osteffen@redhat.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Except two nits below...

> ---
>  grub-core/kern/efi/efi.c | 21 ++++++---------------
>  grub-core/kern/misc.c    | 32 ++++++++++++++++++++++++++++++++
>  include/grub/misc.h      |  3 +++
>  3 files changed, 41 insertions(+), 15 deletions(-)
>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index 8df861a59..8fc3edfae 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -207,15 +207,11 @@ grub_efi_set_variable_with_attributes (const char *var, 
> const grub_guid_t *guid,
>    grub_efi_status_t status;
>    grub_efi_runtime_services_t *r;
>    grub_efi_char16_t *var16;
> -  grub_size_t len, len16;
>
> -  len = grub_strlen (var);
> -  len16 = len * GRUB_MAX_UTF16_PER_UTF8;
> -  var16 = grub_calloc (len16 + 1, sizeof (var16[0]));
> -  if (!var16)
> +  grub_utf8_to_utf16_alloc (var, &var16, NULL);
> +
> +  if (var16 == NULL)
>      return grub_errno;
> -  len16 = grub_utf8_to_utf16 (var16, len16, (grub_uint8_t *) var, len, NULL);
> -  var16[len16] = 0;
>
>    r = grub_efi_system_table->runtime_services;
>
> @@ -250,18 +246,13 @@ grub_efi_get_variable_with_attributes (const char *var,
>    grub_efi_runtime_services_t *r;
>    grub_efi_char16_t *var16;
>    void *data;
> -  grub_size_t len, len16;
>
>    *data_out = NULL;
>    *datasize_out = 0;
>
> -  len = grub_strlen (var);
> -  len16 = len * GRUB_MAX_UTF16_PER_UTF8;
> -  var16 = grub_calloc (len16 + 1, sizeof (var16[0]));
> -  if (!var16)
> -    return GRUB_EFI_OUT_OF_RESOURCES;
> -  len16 = grub_utf8_to_utf16 (var16, len16, (grub_uint8_t *) var, len, NULL);
> -  var16[len16] = 0;
> +  grub_utf8_to_utf16_alloc (var, &var16, NULL);
> +  if (var16 == NULL)
> +    return grub_errno;
>
>    r = grub_efi_system_table->runtime_services;
>
> diff --git a/grub-core/kern/misc.c b/grub-core/kern/misc.c
> index 1a8aff543..b25a65863 100644
> --- a/grub-core/kern/misc.c
> +++ b/grub-core/kern/misc.c
> @@ -25,6 +25,7 @@
>  #include <grub/env.h>
>  #include <grub/i18n.h>
>  #include <grub/types.h>
> +#include <grub/charset.h>
>
>  union printf_arg
>  {
> @@ -1323,6 +1324,37 @@ grub_fatal (const char *fmt, ...)
>    grub_abort ();
>  }
>
> +grub_ssize_t
> +grub_utf8_to_utf16_alloc (const char *str8, grub_uint16_t **utf16_msg, 
> grub_uint16_t **last_position)
> +{
> +  grub_size_t len;
> +  grub_size_t len16;
> +
> +  len = grub_strlen (str8);
> +
> +  /* Check for integer overflow */
> +  if (len > GRUB_SSIZE_MAX / GRUB_MAX_UTF16_PER_UTF8 - 1)
> +    {
> +      grub_error (GRUB_ERR_BAD_ARGUMENT, N_("string too large"));

s/string too large/string too long/?

> +      *utf16_msg = NULL;
> +      return -1;
> +    }
> +
> +  len16 = len * GRUB_MAX_UTF16_PER_UTF8;
> +
> +  *utf16_msg = grub_calloc (len16 + 1, sizeof (*utf16_msg[0]));
> +  if (*utf16_msg == NULL)
> +    return -1;
> +
> +  len16 = grub_utf8_to_utf16 (*utf16_msg, len16, (grub_uint8_t *) str8, len, 
> NULL);
> +
> +  if (last_position)

s/last_position/last_position != NULL/

Daniel