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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Jul 7, 2025 at 4:42 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.
>

Okay, I'll follow the rename

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

Not as far as I know. Why? Some systems even can use NULL pointers as
valid, like mmap.

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

I can use again argc if you prefer, not strong about it.

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

I realized that because I got a crash, not just by looking at the code.

The string was something like "-- a b c d":
- the first get_argv call produces a 5 argc;
- you allocate space for 6 pointers and length of the entire string to copy;
- the parser writes a single pointer in argv and returns still 5 as argc;
- returned argc is ignored;
- code "for (i = 1; i < argc; ++i)" starts accessing argv[1] which is
not initialized, in case of garbage you dereference garbage.

Frediano