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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Jul 08, 2025 at 02:56:58PM +0100, 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.

I wouldn't call it "buffer overflow" - the argv array is big enough
here. But if there is "--" in cmdline, it has fewer than argc elements
initialized. If there is at least one efi option (IOW, "--" is not the
first one), the sentinel NULL inserted by get_argv() will prevent
reading past the initialized part. But if "--" is the first one, the
NULL is inserted into argv[0], which is skipped by the loop in
efi_start(). Which makes the loop go beyond initialized part of argv
(crash happens even before it goes beyond end of argv allocation).

So, maybe change it to: bigger than the initialized portion of "argv"
array, leading to potential uninitialized pointer dereference, ...?

> Using EFI shell is possible to pass any kind of string in
> LoadOptions.
> 
> Fixes: bf6501a62e80 ("x86-64: EFI boot code")

Technically, the issue was covered for few months by another issue and
got re-exposed by 926e680aadde ("EFI: suppress bogus loader warning").
While it fixed one issue, it also made it possible to put sentinel NULL
into argv[0] again. But the original EFI code had this issue too, so
IMO the Fixes tag is correct.

While there is convention to put file name as the first option, I don't
see anything in the UEFI spec requiring it. So, Xen should not crash
when it's missing.

> Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>

With commit message adjusted:
Reviewed-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>

> ---
> Changes since v1:
> - use argc to make code more clear;
> - fix commit reference;
> - improve commit message.
> ---
>  xen/common/efi/boot.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 9306dc8953..385292ad4e 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -350,10 +350,11 @@ static unsigned int __init get_argv(unsigned int argc, 
> CHAR16 **argv,
>  
>      if ( argc )
>      {
> +        argc = 0;
>          cmdline = data + *offset;
>          /* EFI_LOAD_OPTION does not supply an image name as first component. 
> */
>          if ( *offset )
> -            *argv++ = NULL;
> +            argv[argc++] = NULL;
>      }
>      else if ( size > sizeof(*cmdline) && !(size % sizeof(*cmdline)) &&
>                (wmemchr(data, 0, size / sizeof(*cmdline)) ==
> @@ -414,14 +415,14 @@ static unsigned int __init get_argv(unsigned int argc, 
> CHAR16 **argv,
>                  ++argc;
>              else if ( prev && wstrcmp(prev, L"--") == 0 )
>              {
> -                --argv;
> +                --argc;
>                  if ( options )
>                      *options = cmdline;
>                  break;
>              }
>              else
>              {
> -                *argv++ = prev = ptr;
> +                argv[argc++] = prev = ptr;
>                  *ptr = *cmdline;
>                  *++ptr = 0;
>              }
> @@ -429,7 +430,7 @@ static unsigned int __init get_argv(unsigned int argc, 
> CHAR16 **argv,
>          prev_sep = cur_sep;
>      }
>      if ( argv )
> -        *argv = NULL;
> +        argv[argc] = NULL;
>      return argc;
>  }
>  
> @@ -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);
>          else
>              argc = 0;
>          for ( i = 1; i < argc; ++i )
> -- 
> 2.43.0
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature