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

[Jan Beulich <jbeulich@xxxxxxxx>]

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