ping
On Tue, Jul 8, 2025 at 2:57 PM Frediano Ziglio
<frediano.zig...@cloud.com> 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.zig...@cloud.com>
> ---
> 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
>
