Re: [Xen-devel] [PATCH] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Jan 3, 2018 at 9:36 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 03.01.18 at 17:04, <tamas@xxxxxxxxxxxxx> wrote:
>> On Wed, Jan 3, 2018 at 4:20 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>> On 02.01.18 at 16:56, <tamas@xxxxxxxxxxxxx> wrote:
>>>> +        if ( elo->Attributes & LOAD_OPTION_ACTIVE )
>>>
>>> Without any other (earlier) check, how can you reliably tell this
>>> being a pointer to EFI_LOAD_OPTION from it being the
>>> currently used one to CHAR16? Is the distinction perhaps UEFI
>>> version dependent? In the 2.3 spec I can't find any information
>>> on the layout of what ->LoadOptions points to.
>>
>> AFAICT there is no clear cut way to distinguish what is being passed
>> here. When launched via UEFI Xen receives the EFI_LOAD_OPTION buffer
>> here already, which it tries to parse as a string and fails. Take a
>> look at the same problem in the shim:
>> https://github.com/rhboot/shim/blob/master/shim.c#L2501. If there is a
>> clearer way to distinguish what is being passed here or more checks
>> that can be done I would be open for suggestions.
>
> Well, first of all the code you point to tells me that this situation is
> indeed as bad as it can be. However, the code also shows you
> some heuristics you could re-use.

From what I've seen the only heuristic we could copy is counting ucs2
strings in the buffer. I wasn't entirely convinced that it's necessary
and rather went with the "if it looks like a duck, swims like a duck,
and quacks like a duck, then it probably is a duck" test. What are the
chances we could encounter a string that also properly parses as an
EFI_LOAD_OPTION buffer passing the checks in place in this patch?
Anyway, if the ucs2 string counting is something we also want to
utilize, I have nothing against it.

> What they don't seem to utilize
> is the fact that as long as there are few enough bits defined for
> the Attributes field, checking that one for being a printable
> character (and the upper 16 bits to be non-null) might give a good
> indication what form we're dealing with. In fact it might be that
> when it's a string, the first character is always '\', but I'm afraid
> that's not written down anywhere.
>

I don't think that's true, when I printed this buffer when launched
through the shell (after entering the xen folder on the ESP) it
started with "xen.efi".

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel