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

[Jan Beulich <jbeulich@xxxxxxxx>]

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