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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jan 4, 2018 at 3:43 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 03.01.18 at 17:53, <tamas@xxxxxxxxxxxxx> wrote:
>> 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.
>
> Actually that string counting seemed rather fragile, but maybe I
> didn't fully understand it.

That was my impression of it too.

> Just looking at the low bit of the first
> byte before assuming this could be a load option structure,
> however, is too weak a check for my taste.

There is more done in this patch then just checking the low bit of the
Attributes field: calculating where the OptionalData should be if this
is indeed an EFI_LOAD_OPTION and then sanity checking if that address
is sane or not. This requires the buffer to have a layout that at
least parses properly as an EFI_LOAD_OPTION, hence it likely is one..

Tamas

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