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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Tue, Jul 8, 2025 at 4:41 PM Jan Beulich <jbeulich@xxxxxxxx> 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.
>

The only reason I put the "Fixes" comments it's that you always asked
me to do so. From what you wrote it looks like you are taking it
personally. I don't care much why there is the bug or when it was
introduced, I found a crash in Xen and I want to fix it. Marek in
another comment said Xen should not crash that way. IMO even if the
bug turns out to be outside Xen and GRUB should always provide
something as argv[0] Xen is failing validating the input received and
good software should properly validate inputs.
>From a discussion in XenDevel chat Antony pointed out the script at
https://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=overlay-bookworm/etc/grub.d/20_linux_xen;h=8559352563d9a2466670661671f306659ace2590;hb=HEAD#l259.
On line 160 you can see

 ${xen_loader}   ${rel_xen_dirname}/${xen_basename} placeholder
${xen_args} \${xen_rm_opts}

that is a dummy "placeholder" argument is added to replace the
argv[0]. In other words, possibly removing this "issue" from GRUB will
now cause regressions.

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

"argc" and "argv" are usually the command line parameters in a C
program. "argv" is a NULL terminated array so why avoid this coherency
from C?

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

It does not hurt and apparently for EFI_LOAD_OPTION we fill argv[0]
with a NULL instead of an empty or dummy string so I would keep this
test anyway.

>
> just out of context here?
>
> Jan

Frediano