xen/common/efi/boot.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
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@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.0On Tue, Jul 08, 2025 at 02:56:58PM +0100, 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.
I wouldn't call it "buffer overflow" - the argv array is big enough
here. But if there is "--" in cmdline, it has fewer than argc elements
initialized. If there is at least one efi option (IOW, "--" is not the
first one), the sentinel NULL inserted by get_argv() will prevent
reading past the initialized part. But if "--" is the first one, the
NULL is inserted into argv[0], which is skipped by the loop in
efi_start(). Which makes the loop go beyond initialized part of argv
(crash happens even before it goes beyond end of argv allocation).
So, maybe change it to: bigger than the initialized portion of "argv"
array, leading to potential uninitialized pointer dereference, ...?
> Using EFI shell is possible to pass any kind of string in
> LoadOptions.
>
> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
Technically, the issue was covered for few months by another issue and
got re-exposed by 926e680aadde ("EFI: suppress bogus loader warning").
While it fixed one issue, it also made it possible to put sentinel NULL
into argv[0] again. But the original EFI code had this issue too, so
IMO the Fixes tag is correct.
While there is convention to put file name as the first option, I don't
see anything in the UEFI spec requiring it. So, Xen should not crash
when it's missing.
> Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
With commit message adjusted:
Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.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
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
On 29.08.2025 06:06, Marek Marczykowski-Górecki wrote:
> On Tue, Jul 08, 2025 at 02:56:58PM +0100, 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.
>
> I wouldn't call it "buffer overflow" - the argv array is big enough
> here. But if there is "--" in cmdline, it has fewer than argc elements
> initialized. If there is at least one efi option (IOW, "--" is not the
> first one), the sentinel NULL inserted by get_argv() will prevent
> reading past the initialized part. But if "--" is the first one, the
> NULL is inserted into argv[0], which is skipped by the loop in
> efi_start(). Which makes the loop go beyond initialized part of argv
> (crash happens even before it goes beyond end of argv allocation).
>
> So, maybe change it to: bigger than the initialized portion of "argv"
> array, leading to potential uninitialized pointer dereference, ...?
>
>> Using EFI shell is possible to pass any kind of string in
>> LoadOptions.
>>
>> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
>
> Technically, the issue was covered for few months by another issue and
> got re-exposed by 926e680aadde ("EFI: suppress bogus loader warning").
> While it fixed one issue, it also made it possible to put sentinel NULL
> into argv[0] again. But the original EFI code had this issue too, so
> IMO the Fixes tag is correct.
>
> While there is convention to put file name as the first option, I don't
> see anything in the UEFI spec requiring it. So, Xen should not crash
> when it's missing.
Yet if the equivalent of argv[0] is missing from the command line, how
do we even know whether the first token on the command line is an
option (or the -- separator)?
Jan
On Fri, Aug 29, 2025 at 09:17:31AM +0200, Jan Beulich wrote:
> On 29.08.2025 06:06, Marek Marczykowski-Górecki wrote:
> > On Tue, Jul 08, 2025 at 02:56:58PM +0100, 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.
> >
> > I wouldn't call it "buffer overflow" - the argv array is big enough
> > here. But if there is "--" in cmdline, it has fewer than argc elements
> > initialized. If there is at least one efi option (IOW, "--" is not the
> > first one), the sentinel NULL inserted by get_argv() will prevent
> > reading past the initialized part. But if "--" is the first one, the
> > NULL is inserted into argv[0], which is skipped by the loop in
> > efi_start(). Which makes the loop go beyond initialized part of argv
> > (crash happens even before it goes beyond end of argv allocation).
> >
> > So, maybe change it to: bigger than the initialized portion of "argv"
> > array, leading to potential uninitialized pointer dereference, ...?
> >
> >> Using EFI shell is possible to pass any kind of string in
> >> LoadOptions.
> >>
> >> Fixes: bf6501a62e80 ("x86-64: EFI boot code")
> >
> > Technically, the issue was covered for few months by another issue and
> > got re-exposed by 926e680aadde ("EFI: suppress bogus loader warning").
> > While it fixed one issue, it also made it possible to put sentinel NULL
> > into argv[0] again. But the original EFI code had this issue too, so
> > IMO the Fixes tag is correct.
> >
> > While there is convention to put file name as the first option, I don't
> > see anything in the UEFI spec requiring it. So, Xen should not crash
> > when it's missing.
>
> Yet if the equivalent of argv[0] is missing from the command line, how
> do we even know whether the first token on the command line is an
> option (or the -- separator)?
I think that assumption of argv[0] presence in LoadOptions was always
buggy... But at this point everybody is adding "placeholder" as the
first argument anyway (which predates EFI code in Xen), so we can very
well continue to require it, at least in this release.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
ping
On Tue, Jul 8, 2025 at 2:57 PM Frediano Ziglio
<frediano.ziglio@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.ziglio@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
>
ping
On Mon, Jul 28, 2025 at 11:39 AM Frediano Ziglio
<frediano.ziglio@cloud.com> wrote:
>
> ping
>
> On Tue, Jul 8, 2025 at 2:57 PM Frediano Ziglio
> <frediano.ziglio@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.ziglio@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
> >
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
On Tue, Jul 8, 2025 at 4:41 PM Jan Beulich <jbeulich@suse.com> 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
On 09.07.2025 11:07, Frediano Ziglio wrote:
> On Tue, Jul 8, 2025 at 4:41 PM Jan Beulich <jbeulich@suse.com> 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.
Yes, such an issue wants fixing. But it's also relevant to understand
whether this is a workaround for something, or whether our code was
genuinely broken. In the latter case I'd further learn from that, whatever
it was that I didn't pay attention to back then. The EFI maintainers may
well view this differently, and it is them to eventually approve the patch
in whatever shape or form.
Jan
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
On 08.07.2025 19:22, Marek Marczykowski-Górecki wrote:
> 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.
This command line as written by the user isn't invalid. But what it is
transformed to when passed to the image entry point is bogus, at least
according to my understanding. It's not the user after all who should
add the image name as first component. The image name being first is
a very conventional thing in argc/argv land (which includes plain
command lines like in the Windows world, which in turn is what I
understand EFI derived from), after all, so it being like that may not
even need explicitly spelling out anywhere.
Jan
© 2016 - 2025 Red Hat, Inc.