Re: [PATCH] xen/efi: Fix crash with initial empty EFI options

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.07.2025 17:11, Frediano Ziglio wrote:
> EFI code path split options from EFI LoadOptions fields in 2
> pieces, first EFI options, second Xen options.
> "get_argv" function is called first to get the number of arguments
> in the LoadOptions, second, after allocating enough space, to
> fill some "argc"/"argv" variable. However the first parsing could
> be different from second as second is able to detect "--" argument
> separator. So it was possible that "argc" was bigger that the "argv"
> array leading to potential buffer overflows, in particular
> a string like "-- a b c" would lead to buffer overflow in "argv"
> resulting in crashes.
> Using EFI shell is possible to pass any kind of string in
> LoadOptions.
> 
> Fixes: 201f261e859e ("EFI: move x86 boot/runtime code to common/efi")

This only moves the function, but doesn't really introduce any issue afaics.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -345,6 +345,7 @@ static unsigned int __init get_argv(unsigned int argc, 
> CHAR16 **argv,
>                                      VOID *data, UINTN size, UINTN *offset,
>                                      CHAR16 **options)
>  {
> +    CHAR16 **const orig_argv = argv;
>      CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL;
>      bool prev_sep = true;
>  
> @@ -384,7 +385,7 @@ static unsigned int __init get_argv(unsigned int argc, 
> CHAR16 **argv,
>                  {
>                      cmdline = data + *offset;
>                      /* Cater for the image name as first component. */
> -                    ++argc;
> +                    ++argv;

We're on the argc == 0 and argv == NULL path here. Incrementing NULL is UB,
if I'm not mistaken.

> @@ -402,7 +403,7 @@ static unsigned int __init get_argv(unsigned int argc, 
> CHAR16 **argv,
>          {
>              if ( cur_sep )
>                  ++ptr;
> -            else if ( argv )
> +            else if ( orig_argv )
>              {
>                  *ptr = *cmdline;
>                  *++ptr = 0;
> @@ -410,8 +411,8 @@ static unsigned int __init get_argv(unsigned int argc, 
> CHAR16 **argv,
>          }
>          else if ( !cur_sep )
>          {
> -            if ( !argv )
> -                ++argc;
> +            if ( !orig_argv )
> +                ++argv;
>              else if ( prev && wstrcmp(prev, L"--") == 0 )
>              {
>                  --argv;

As per this, it looks like that on the 1st pass we may indeed overcount
arguments. But ...

> @@ -428,9 +429,9 @@ static unsigned int __init get_argv(unsigned int argc, 
> CHAR16 **argv,
>          }
>          prev_sep = cur_sep;
>      }
> -    if ( argv )
> +    if ( orig_argv )
>          *argv = NULL;
> -    return argc;
> +    return argv - orig_argv;
>  }
>  
>  static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE 
> *loaded_image,
> @@ -1348,8 +1349,8 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE 
> ImageHandle,
>                                    (argc + 1) * sizeof(*argv) +
>                                        loaded_image->LoadOptionsSize,
>                                    (void **)&argv) == EFI_SUCCESS )
> -            get_argv(argc, argv, loaded_image->LoadOptions,
> -                     loaded_image->LoadOptionsSize, &offset, &options);
> +            argc = get_argv(argc, argv, loaded_image->LoadOptions,
> +                            loaded_image->LoadOptionsSize, &offset, 
> &options);

... wouldn't this change alone cure that problem? And even that I don't
follow. Below here we have

        for ( i = 1; i < argc; ++i )
        {
            CHAR16 *ptr = argv[i];

            if ( !ptr )
                break;

and the 2nd pass of get_argv() properly terminates the (possibly too large)
array with a NULL sentinel. So I wonder what it is that I'm overlooking and
that is broken.

Jan