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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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

Attachment: signature.asc
Description: PGP signature