xen/common/efi/boot.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 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: 201f261e859e ("EFI: move x86 boot/runtime code to common/efi")
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
xen/common/efi/boot.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 9306dc8953..597252cfc4 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -345,6 +345,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
VOID *data, UINTN size, UINTN *offset,
CHAR16 **options)
{
+ CHAR16 **const orig_argv = argv;
CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL;
bool prev_sep = true;
@@ -384,7 +385,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
{
cmdline = data + *offset;
/* Cater for the image name as first component. */
- ++argc;
+ ++argv;
}
}
}
@@ -402,7 +403,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
{
if ( cur_sep )
++ptr;
- else if ( argv )
+ else if ( orig_argv )
{
*ptr = *cmdline;
*++ptr = 0;
@@ -410,8 +411,8 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
}
else if ( !cur_sep )
{
- if ( !argv )
- ++argc;
+ if ( !orig_argv )
+ ++argv;
else if ( prev && wstrcmp(prev, L"--") == 0 )
{
--argv;
@@ -428,9 +429,9 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
}
prev_sep = cur_sep;
}
- if ( argv )
+ if ( orig_argv )
*argv = NULL;
- return argc;
+ return argv - orig_argv;
}
static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_image,
@@ -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 07.07.2025 17:11, 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: 201f261e859e ("EFI: move x86 boot/runtime code to common/efi")
This only moves the function, but doesn't really introduce any issue afaics.
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -345,6 +345,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> VOID *data, UINTN size, UINTN *offset,
> CHAR16 **options)
> {
> + CHAR16 **const orig_argv = argv;
> CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL;
> bool prev_sep = true;
>
> @@ -384,7 +385,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> {
> cmdline = data + *offset;
> /* Cater for the image name as first component. */
> - ++argc;
> + ++argv;
We're on the argc == 0 and argv == NULL path here. Incrementing NULL is UB,
if I'm not mistaken.
> @@ -402,7 +403,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> {
> if ( cur_sep )
> ++ptr;
> - else if ( argv )
> + else if ( orig_argv )
> {
> *ptr = *cmdline;
> *++ptr = 0;
> @@ -410,8 +411,8 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> }
> else if ( !cur_sep )
> {
> - if ( !argv )
> - ++argc;
> + if ( !orig_argv )
> + ++argv;
> else if ( prev && wstrcmp(prev, L"--") == 0 )
> {
> --argv;
As per this, it looks like that on the 1st pass we may indeed overcount
arguments. But ...
> @@ -428,9 +429,9 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> }
> prev_sep = cur_sep;
> }
> - if ( argv )
> + if ( orig_argv )
> *argv = NULL;
> - return argc;
> + return argv - orig_argv;
> }
>
> static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_image,
> @@ -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);
... wouldn't this change alone cure that problem? And even that I don't
follow. Below here we have
for ( i = 1; i < argc; ++i )
{
CHAR16 *ptr = argv[i];
if ( !ptr )
break;
and the 2nd pass of get_argv() properly terminates the (possibly too large)
array with a NULL sentinel. So I wonder what it is that I'm overlooking and
that is broken.
Jan
On Mon, Jul 7, 2025 at 4:42 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 07.07.2025 17:11, 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: 201f261e859e ("EFI: move x86 boot/runtime code to common/efi")
>
> This only moves the function, but doesn't really introduce any issue afaics.
>
Okay, I'll follow the rename
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -345,6 +345,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> > VOID *data, UINTN size, UINTN *offset,
> > CHAR16 **options)
> > {
> > + CHAR16 **const orig_argv = argv;
> > CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL;
> > bool prev_sep = true;
> >
> > @@ -384,7 +385,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> > {
> > cmdline = data + *offset;
> > /* Cater for the image name as first component. */
> > - ++argc;
> > + ++argv;
>
> We're on the argc == 0 and argv == NULL path here. Incrementing NULL is UB,
> if I'm not mistaken.
>
Not as far as I know. Why? Some systems even can use NULL pointers as
valid, like mmap.
> > @@ -402,7 +403,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> > {
> > if ( cur_sep )
> > ++ptr;
> > - else if ( argv )
> > + else if ( orig_argv )
> > {
> > *ptr = *cmdline;
> > *++ptr = 0;
> > @@ -410,8 +411,8 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> > }
> > else if ( !cur_sep )
> > {
> > - if ( !argv )
> > - ++argc;
> > + if ( !orig_argv )
> > + ++argv;
> > else if ( prev && wstrcmp(prev, L"--") == 0 )
> > {
> > --argv;
>
> As per this, it looks like that on the 1st pass we may indeed overcount
> arguments. But ...
>
I can use again argc if you prefer, not strong about it.
> > @@ -428,9 +429,9 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> > }
> > prev_sep = cur_sep;
> > }
> > - if ( argv )
> > + if ( orig_argv )
> > *argv = NULL;
> > - return argc;
> > + return argv - orig_argv;
> > }
> >
> > static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_image,
> > @@ -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);
>
> ... wouldn't this change alone cure that problem? And even that I don't
> follow. Below here we have
>
> for ( i = 1; i < argc; ++i )
> {
> CHAR16 *ptr = argv[i];
>
> if ( !ptr )
> break;
>
> and the 2nd pass of get_argv() properly terminates the (possibly too large)
> array with a NULL sentinel. So I wonder what it is that I'm overlooking and
> that is broken.
>
> Jan
I realized that because I got a crash, not just by looking at the code.
The string was something like "-- a b c d":
- the first get_argv call produces a 5 argc;
- you allocate space for 6 pointers and length of the entire string to copy;
- the parser writes a single pointer in argv and returns still 5 as argc;
- returned argc is ignored;
- code "for (i = 1; i < argc; ++i)" starts accessing argv[1] which is
not initialized, in case of garbage you dereference garbage.
Frediano
On 07.07.2025 17:51, Frediano Ziglio wrote:
> On Mon, Jul 7, 2025 at 4:42 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 07.07.2025 17:11, 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: 201f261e859e ("EFI: move x86 boot/runtime code to common/efi")
>>
>> This only moves the function, but doesn't really introduce any issue afaics.
>>
>
> Okay, I'll follow the rename
>
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -345,6 +345,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>> VOID *data, UINTN size, UINTN *offset,
>>> CHAR16 **options)
>>> {
>>> + CHAR16 **const orig_argv = argv;
>>> CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL;
>>> bool prev_sep = true;
>>>
>>> @@ -384,7 +385,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>> {
>>> cmdline = data + *offset;
>>> /* Cater for the image name as first component. */
>>> - ++argc;
>>> + ++argv;
>>
>> We're on the argc == 0 and argv == NULL path here. Incrementing NULL is UB,
>> if I'm not mistaken.
>
> Not as far as I know. Why?
Increment and decrement operators are like additions. For additions the standard
says: "For addition, either both operands shall have arithmetic type, or one
operand shall be a pointer to an object type and the other shall have integer
type." Neither of the alternatives is true for NULL.
> Some systems even can use NULL pointers as valid, like mmap.
Right, but that doesn't make the use of NULL C-compliant.
>>> @@ -402,7 +403,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>> {
>>> if ( cur_sep )
>>> ++ptr;
>>> - else if ( argv )
>>> + else if ( orig_argv )
>>> {
>>> *ptr = *cmdline;
>>> *++ptr = 0;
>>> @@ -410,8 +411,8 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>> }
>>> else if ( !cur_sep )
>>> {
>>> - if ( !argv )
>>> - ++argc;
>>> + if ( !orig_argv )
>>> + ++argv;
>>> else if ( prev && wstrcmp(prev, L"--") == 0 )
>>> {
>>> --argv;
>>
>> As per this, it looks like that on the 1st pass we may indeed overcount
>> arguments. But ...
>>
>
> I can use again argc if you prefer, not strong about it.
>
>>> @@ -428,9 +429,9 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>> }
>>> prev_sep = cur_sep;
>>> }
>>> - if ( argv )
>>> + if ( orig_argv )
>>> *argv = NULL;
>>> - return argc;
>>> + return argv - orig_argv;
>>> }
>>>
>>> static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_image,
>>> @@ -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);
>>
>> ... wouldn't this change alone cure that problem? And even that I don't
>> follow. Below here we have
>>
>> for ( i = 1; i < argc; ++i )
>> {
>> CHAR16 *ptr = argv[i];
>>
>> if ( !ptr )
>> break;
>>
>> and the 2nd pass of get_argv() properly terminates the (possibly too large)
>> array with a NULL sentinel. So I wonder what it is that I'm overlooking and
>> that is broken.
>
> I realized that because I got a crash, not just by looking at the code.
>
> The string was something like "-- a b c d":
That's in the "plain command line" case or the LOAD_OPTIONS one? In the
former case the image name should come first, aiui. And in the latter case
the 2nd pass sets argv[0] to NULL very early, increments the pointer, and
hence at the bottom of the function argv[1] would also be set to NULL.
Aiui at least, i.e. ...
> - the first get_argv call produces a 5 argc;
> - you allocate space for 6 pointers and length of the entire string to copy;
> - the parser writes a single pointer in argv and returns still 5 as argc;
> - returned argc is ignored;
> - code "for (i = 1; i < argc; ++i)" starts accessing argv[1] which is
> not initialized, in case of garbage you dereference garbage.
... I don't see how argv[1] can hold garbage.
Jan
On Mon, Jul 7, 2025 at 5:04 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 07.07.2025 17:51, Frediano Ziglio wrote:
> > On Mon, Jul 7, 2025 at 4:42 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 07.07.2025 17:11, 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: 201f261e859e ("EFI: move x86 boot/runtime code to common/efi")
> >>
> >> This only moves the function, but doesn't really introduce any issue afaics.
> >>
> >
> > Okay, I'll follow the rename
> >
> >>> --- a/xen/common/efi/boot.c
> >>> +++ b/xen/common/efi/boot.c
> >>> @@ -345,6 +345,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>> VOID *data, UINTN size, UINTN *offset,
> >>> CHAR16 **options)
> >>> {
> >>> + CHAR16 **const orig_argv = argv;
> >>> CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL;
> >>> bool prev_sep = true;
> >>>
> >>> @@ -384,7 +385,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>> {
> >>> cmdline = data + *offset;
> >>> /* Cater for the image name as first component. */
> >>> - ++argc;
> >>> + ++argv;
> >>
> >> We're on the argc == 0 and argv == NULL path here. Incrementing NULL is UB,
> >> if I'm not mistaken.
> >
> > Not as far as I know. Why?
>
> Increment and decrement operators are like additions. For additions the standard
> says: "For addition, either both operands shall have arithmetic type, or one
> operand shall be a pointer to an object type and the other shall have integer
> type." Neither of the alternatives is true for NULL.
>
Yes and no. The expression here is not NULL + 1, but (CHAR16**)NULL +
1, hence the pointer has a type and so the expression is valid.
> > Some systems even can use NULL pointers as valid, like mmap.
>
> Right, but that doesn't make the use of NULL C-compliant.
>
> >>> @@ -402,7 +403,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>> {
> >>> if ( cur_sep )
> >>> ++ptr;
> >>> - else if ( argv )
> >>> + else if ( orig_argv )
> >>> {
> >>> *ptr = *cmdline;
> >>> *++ptr = 0;
> >>> @@ -410,8 +411,8 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>> }
> >>> else if ( !cur_sep )
> >>> {
> >>> - if ( !argv )
> >>> - ++argc;
> >>> + if ( !orig_argv )
> >>> + ++argv;
> >>> else if ( prev && wstrcmp(prev, L"--") == 0 )
> >>> {
> >>> --argv;
> >>
> >> As per this, it looks like that on the 1st pass we may indeed overcount
> >> arguments. But ...
> >>
> >
> > I can use again argc if you prefer, not strong about it.
> >
> >>> @@ -428,9 +429,9 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>> }
> >>> prev_sep = cur_sep;
> >>> }
> >>> - if ( argv )
> >>> + if ( orig_argv )
> >>> *argv = NULL;
> >>> - return argc;
> >>> + return argv - orig_argv;
> >>> }
> >>>
> >>> static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_image,
> >>> @@ -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);
> >>
> >> ... wouldn't this change alone cure that problem? And even that I don't
> >> follow. Below here we have
> >>
> >> for ( i = 1; i < argc; ++i )
> >> {
> >> CHAR16 *ptr = argv[i];
> >>
> >> if ( !ptr )
> >> break;
> >>
> >> and the 2nd pass of get_argv() properly terminates the (possibly too large)
> >> array with a NULL sentinel. So I wonder what it is that I'm overlooking and
> >> that is broken.
> >
> > I realized that because I got a crash, not just by looking at the code.
> >
> > The string was something like "-- a b c d":
>
> That's in the "plain command line" case or the LOAD_OPTIONS one? In the
> former case the image name should come first, aiui. And in the latter case
> the 2nd pass sets argv[0] to NULL very early, increments the pointer, and
> hence at the bottom of the function argv[1] would also be set to NULL.
> Aiui at least, i.e. ...
>
> > - the first get_argv call produces a 5 argc;
> > - you allocate space for 6 pointers and length of the entire string to copy;
> > - the parser writes a single pointer in argv and returns still 5 as argc;
> > - returned argc is ignored;
> > - code "for (i = 1; i < argc; ++i)" starts accessing argv[1] which is
> > not initialized, in case of garbage you dereference garbage.
>
> ... I don't see how argv[1] can hold garbage.
>
As I said, this happened as a crash during testing, not looking at the
code. It's a plain string in LoadOptions, *offset is set to 0 so
there's no initial set of argv[0]. argv[0] is set with the beginning
of "--" but then when "--" is detected" argv is moved back to initial
value and the terminator is written still in argv[0], so argv[1] is
never written.
> Jan
Frediano
On 08.07.2025 08:03, Frediano Ziglio wrote:
> On Mon, Jul 7, 2025 at 5:04 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 07.07.2025 17:51, Frediano Ziglio wrote:
>>> On Mon, Jul 7, 2025 at 4:42 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 07.07.2025 17:11, Frediano Ziglio wrote:
>>>>> --- a/xen/common/efi/boot.c
>>>>> +++ b/xen/common/efi/boot.c
>>>>> @@ -345,6 +345,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>>>> VOID *data, UINTN size, UINTN *offset,
>>>>> CHAR16 **options)
>>>>> {
>>>>> + CHAR16 **const orig_argv = argv;
>>>>> CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL;
>>>>> bool prev_sep = true;
>>>>>
>>>>> @@ -384,7 +385,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>>>> {
>>>>> cmdline = data + *offset;
>>>>> /* Cater for the image name as first component. */
>>>>> - ++argc;
>>>>> + ++argv;
>>>>
>>>> We're on the argc == 0 and argv == NULL path here. Incrementing NULL is UB,
>>>> if I'm not mistaken.
>>>
>>> Not as far as I know. Why?
>>
>> Increment and decrement operators are like additions. For additions the standard
>> says: "For addition, either both operands shall have arithmetic type, or one
>> operand shall be a pointer to an object type and the other shall have integer
>> type." Neither of the alternatives is true for NULL.
>
> Yes and no. The expression here is not NULL + 1, but (CHAR16**)NULL +
> 1, hence the pointer has a type and so the expression is valid.
Sorry, meant to reply to this as well: That's not my understanding of the word
"object".
Jan
On 08.07.2025 08:03, Frediano Ziglio wrote:
> On Mon, Jul 7, 2025 at 5:04 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 07.07.2025 17:51, Frediano Ziglio wrote:
>>> On Mon, Jul 7, 2025 at 4:42 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 07.07.2025 17:11, 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: 201f261e859e ("EFI: move x86 boot/runtime code to common/efi")
>>>>
>>>> This only moves the function, but doesn't really introduce any issue afaics.
>>>>
>>>
>>> Okay, I'll follow the rename
>>>
>>>>> --- a/xen/common/efi/boot.c
>>>>> +++ b/xen/common/efi/boot.c
>>>>> @@ -345,6 +345,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>>>> VOID *data, UINTN size, UINTN *offset,
>>>>> CHAR16 **options)
>>>>> {
>>>>> + CHAR16 **const orig_argv = argv;
>>>>> CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL;
>>>>> bool prev_sep = true;
>>>>>
>>>>> @@ -384,7 +385,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>>>> {
>>>>> cmdline = data + *offset;
>>>>> /* Cater for the image name as first component. */
>>>>> - ++argc;
>>>>> + ++argv;
>>>>
>>>> We're on the argc == 0 and argv == NULL path here. Incrementing NULL is UB,
>>>> if I'm not mistaken.
>>>
>>> Not as far as I know. Why?
>>
>> Increment and decrement operators are like additions. For additions the standard
>> says: "For addition, either both operands shall have arithmetic type, or one
>> operand shall be a pointer to an object type and the other shall have integer
>> type." Neither of the alternatives is true for NULL.
>>
>
> Yes and no. The expression here is not NULL + 1, but (CHAR16**)NULL +
> 1, hence the pointer has a type and so the expression is valid.
>
>>> Some systems even can use NULL pointers as valid, like mmap.
>>
>> Right, but that doesn't make the use of NULL C-compliant.
>>
>>>>> @@ -402,7 +403,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>>>> {
>>>>> if ( cur_sep )
>>>>> ++ptr;
>>>>> - else if ( argv )
>>>>> + else if ( orig_argv )
>>>>> {
>>>>> *ptr = *cmdline;
>>>>> *++ptr = 0;
>>>>> @@ -410,8 +411,8 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>>>> }
>>>>> else if ( !cur_sep )
>>>>> {
>>>>> - if ( !argv )
>>>>> - ++argc;
>>>>> + if ( !orig_argv )
>>>>> + ++argv;
>>>>> else if ( prev && wstrcmp(prev, L"--") == 0 )
>>>>> {
>>>>> --argv;
>>>>
>>>> As per this, it looks like that on the 1st pass we may indeed overcount
>>>> arguments. But ...
>>>>
>>>
>>> I can use again argc if you prefer, not strong about it.
>>>
>>>>> @@ -428,9 +429,9 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>>>> }
>>>>> prev_sep = cur_sep;
>>>>> }
>>>>> - if ( argv )
>>>>> + if ( orig_argv )
>>>>> *argv = NULL;
>>>>> - return argc;
>>>>> + return argv - orig_argv;
>>>>> }
>>>>>
>>>>> static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_image,
>>>>> @@ -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);
>>>>
>>>> ... wouldn't this change alone cure that problem? And even that I don't
>>>> follow. Below here we have
>>>>
>>>> for ( i = 1; i < argc; ++i )
>>>> {
>>>> CHAR16 *ptr = argv[i];
>>>>
>>>> if ( !ptr )
>>>> break;
>>>>
>>>> and the 2nd pass of get_argv() properly terminates the (possibly too large)
>>>> array with a NULL sentinel. So I wonder what it is that I'm overlooking and
>>>> that is broken.
>>>
>>> I realized that because I got a crash, not just by looking at the code.
>>>
>>> The string was something like "-- a b c d":
>>
>> That's in the "plain command line" case or the LOAD_OPTIONS one? In the
>> former case the image name should come first, aiui. And in the latter case
>> the 2nd pass sets argv[0] to NULL very early, increments the pointer, and
>> hence at the bottom of the function argv[1] would also be set to NULL.
>> Aiui at least, i.e. ...
>>
>>> - the first get_argv call produces a 5 argc;
>>> - you allocate space for 6 pointers and length of the entire string to copy;
>>> - the parser writes a single pointer in argv and returns still 5 as argc;
>>> - returned argc is ignored;
>>> - code "for (i = 1; i < argc; ++i)" starts accessing argv[1] which is
>>> not initialized, in case of garbage you dereference garbage.
>>
>> ... I don't see how argv[1] can hold garbage.
>
> As I said, this happened as a crash during testing, not looking at the
> code. It's a plain string in LoadOptions, *offset is set to 0 so
> there's no initial set of argv[0]. argv[0] is set with the beginning
> of "--" but then when "--" is detected" argv is moved back to initial
> value and the terminator is written still in argv[0], so argv[1] is
> never written.
On the 1st pass, which path does get_argv() take? The one commented "Plain
command line, as usually passed by the EFI shell", or the EFI_LOAD_OPTION
one? From your reply above I suspect the former, but then the image name
is missing from that line. Which would look like a firmware bug then, and
hence (if so) would also want describing as such (which in particular
would mean no Fixes: tag).
I'm routinely running xen.efi from the EFI shell on at least two systems,
and I have never had any trouble passing "--" as the first option. Which
I don't do all the time, but every now and then a need for doing so did
arise.
Jan
On Tue, Jul 8, 2025 at 9:26 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.07.2025 08:03, Frediano Ziglio wrote:
> > On Mon, Jul 7, 2025 at 5:04 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 07.07.2025 17:51, Frediano Ziglio wrote:
> >>> On Mon, Jul 7, 2025 at 4:42 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 07.07.2025 17:11, 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: 201f261e859e ("EFI: move x86 boot/runtime code to common/efi")
> >>>>
> >>>> This only moves the function, but doesn't really introduce any issue afaics.
> >>>>
> >>>
> >>> Okay, I'll follow the rename
> >>>
> >>>>> --- a/xen/common/efi/boot.c
> >>>>> +++ b/xen/common/efi/boot.c
> >>>>> @@ -345,6 +345,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>>>> VOID *data, UINTN size, UINTN *offset,
> >>>>> CHAR16 **options)
> >>>>> {
> >>>>> + CHAR16 **const orig_argv = argv;
> >>>>> CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL, *cmdline = NULL;
> >>>>> bool prev_sep = true;
> >>>>>
> >>>>> @@ -384,7 +385,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>>>> {
> >>>>> cmdline = data + *offset;
> >>>>> /* Cater for the image name as first component. */
> >>>>> - ++argc;
> >>>>> + ++argv;
> >>>>
> >>>> We're on the argc == 0 and argv == NULL path here. Incrementing NULL is UB,
> >>>> if I'm not mistaken.
> >>>
> >>> Not as far as I know. Why?
> >>
> >> Increment and decrement operators are like additions. For additions the standard
> >> says: "For addition, either both operands shall have arithmetic type, or one
> >> operand shall be a pointer to an object type and the other shall have integer
> >> type." Neither of the alternatives is true for NULL.
> >>
> >
> > Yes and no. The expression here is not NULL + 1, but (CHAR16**)NULL +
> > 1, hence the pointer has a type and so the expression is valid.
> >
> >>> Some systems even can use NULL pointers as valid, like mmap.
> >>
> >> Right, but that doesn't make the use of NULL C-compliant.
> >>
> >>>>> @@ -402,7 +403,7 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>>>> {
> >>>>> if ( cur_sep )
> >>>>> ++ptr;
> >>>>> - else if ( argv )
> >>>>> + else if ( orig_argv )
> >>>>> {
> >>>>> *ptr = *cmdline;
> >>>>> *++ptr = 0;
> >>>>> @@ -410,8 +411,8 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>>>> }
> >>>>> else if ( !cur_sep )
> >>>>> {
> >>>>> - if ( !argv )
> >>>>> - ++argc;
> >>>>> + if ( !orig_argv )
> >>>>> + ++argv;
> >>>>> else if ( prev && wstrcmp(prev, L"--") == 0 )
> >>>>> {
> >>>>> --argv;
> >>>>
> >>>> As per this, it looks like that on the 1st pass we may indeed overcount
> >>>> arguments. But ...
> >>>>
> >>>
> >>> I can use again argc if you prefer, not strong about it.
> >>>
> >>>>> @@ -428,9 +429,9 @@ static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
> >>>>> }
> >>>>> prev_sep = cur_sep;
> >>>>> }
> >>>>> - if ( argv )
> >>>>> + if ( orig_argv )
> >>>>> *argv = NULL;
> >>>>> - return argc;
> >>>>> + return argv - orig_argv;
> >>>>> }
> >>>>>
> >>>>> static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_image,
> >>>>> @@ -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);
> >>>>
> >>>> ... wouldn't this change alone cure that problem? And even that I don't
> >>>> follow. Below here we have
> >>>>
> >>>> for ( i = 1; i < argc; ++i )
> >>>> {
> >>>> CHAR16 *ptr = argv[i];
> >>>>
> >>>> if ( !ptr )
> >>>> break;
> >>>>
> >>>> and the 2nd pass of get_argv() properly terminates the (possibly too large)
> >>>> array with a NULL sentinel. So I wonder what it is that I'm overlooking and
> >>>> that is broken.
> >>>
> >>> I realized that because I got a crash, not just by looking at the code.
> >>>
> >>> The string was something like "-- a b c d":
> >>
> >> That's in the "plain command line" case or the LOAD_OPTIONS one? In the
> >> former case the image name should come first, aiui. And in the latter case
> >> the 2nd pass sets argv[0] to NULL very early, increments the pointer, and
> >> hence at the bottom of the function argv[1] would also be set to NULL.
> >> Aiui at least, i.e. ...
> >>
> >>> - the first get_argv call produces a 5 argc;
> >>> - you allocate space for 6 pointers and length of the entire string to copy;
> >>> - the parser writes a single pointer in argv and returns still 5 as argc;
> >>> - returned argc is ignored;
> >>> - code "for (i = 1; i < argc; ++i)" starts accessing argv[1] which is
> >>> not initialized, in case of garbage you dereference garbage.
> >>
> >> ... I don't see how argv[1] can hold garbage.
> >
> > As I said, this happened as a crash during testing, not looking at the
> > code. It's a plain string in LoadOptions, *offset is set to 0 so
> > there's no initial set of argv[0]. argv[0] is set with the beginning
> > of "--" but then when "--" is detected" argv is moved back to initial
> > value and the terminator is written still in argv[0], so argv[1] is
> > never written.
>
> On the 1st pass, which path does get_argv() take? The one commented "Plain
> command line, as usually passed by the EFI shell", or the EFI_LOAD_OPTION
> one? From your reply above I suspect the former, but then the image name
> is missing from that line. Which would look like a firmware bug then, and
> hence (if so) would also want describing as such (which in particular
> would mean no Fixes: tag).
>
I managed to reproduce this issue using GRUB commands, specifically
chainloader and xen_hypervisor.
> I'm routinely running xen.efi from the EFI shell on at least two systems,
> and I have never had any trouble passing "--" as the first option. Which
> I don't do all the time, but every now and then a need for doing so did
> arise.
>
> Jan
Frediano
© 2016 - 2025 Red Hat, Inc.