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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

ping

On Mon, Jul 28, 2025 at 11:39 AM Frediano Ziglio
<frediano.ziglio@xxxxxxxxx> wrote:
>
> ping
>
> On Tue, Jul 8, 2025 at 2:57 PM Frediano Ziglio
> <frediano.ziglio@xxxxxxxxx> 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: bf6501a62e80 ("x86-64: EFI boot code")
> > Signed-off-by: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
> > ---
> > 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
> >