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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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