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

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

On Tue, Jul 08, 2025 at 05:41:07PM +0200, Jan Beulich wrote:
> On 08.07.2025 15:56, 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: bf6501a62e80 ("x86-64: EFI boot code")
> 
> Have you convinced yourself that your change isn't a workaround for a
> bug elsewhere? You said you repro-ed with grub's chainloader, but that
> doesn't imply things are being got correct there. I can certainly
> accept that I may have screwed up back then. But I'd like to understand
> what the mistake was, and so far I don't see any. As before, being
> passed just "-- a b c" looks like a bug in the code generating the
> command line.

Even if that's invalid command line (is it? what if you want to pass
only options to dom0, but not to xen itself?), it shouldn't result in a
crash but in an error message.

> > @@ -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;
> 
> Strictly speaking the need for this sentinel now disappears, doesn't it?
> As does ...
> 
> >      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 )
> 
> ... the need for
> 
>             if ( !ptr )
>                 break;
> 
> just out of context here?
> 
> Jan

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

Attachment: signature.asc
Description: PGP signature