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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

Patch ping. Jan, I would like to touch base once more to see if we can
get this patch included in 4.11. The patch as-is correctly tells the
difference between buffers provided by both an EFI shell or by the
firmware as an EFI_LOAD_OPTION.

Thanks,
Tamas

On Wed, Feb 7, 2018 at 9:00 AM, Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
> On Mon, Jan 29, 2018 at 2:18 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 26.01.18 at 18:35, <tamas@xxxxxxxxxxxxx> wrote:
>>> On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>>>> On 23.01.18 at 01:21, <tamas@xxxxxxxxxxxxx> wrote:
>>>>> @@ -375,12 +385,39 @@ static void __init PrintErrMesg(const CHAR16 *mesg, 
>>>>> EFI_STATUS ErrCode)
>>>>>
>>>>>  static unsigned int __init get_argv(unsigned int argc, CHAR16 **argv,
>>>>>                                      CHAR16 *cmdline, UINTN cmdsize,
>>>>> -                                    CHAR16 **options)
>>>>> +                                    CHAR16 **options, bool *elo_active)
>>>>>  {
>>>>>      CHAR16 *ptr = (CHAR16 *)(argv + argc + 1), *prev = NULL;
>>>>>      bool prev_sep = true;
>>>>>
>>>>> -    for ( ; cmdsize > sizeof(*cmdline) && *cmdline;
>>>>> +    if ( cmdsize > sizeof(EFI_LOAD_OPTION) &&
>>>>> +         *(CHAR16 *)((void *)cmdline + cmdsize - sizeof(*cmdline)) != 
>>>>> L'\0' )
>>>>
>>>> This is too lax - you should check whether the nul at that position
>>>> indeed is the _first_ one.
>>>
>>> IMHO that check you suggest has nothing to do with EFI_LOAD_OPTION
>>> support. That's sanity checking a command line buffer. It could
>>> certainly be done, but I would say that belongs in a separate patch.
>>> This check currently as is distinguishes an EFI_LOAD_OPTION from a
>>> well-formed command line buffer. If the command line buffer has
>>> multiple '\0' in it, that's a separate problem.
>>
>> You could view it as a separate problem if there was a non-heuristic
>> way of distinguishing the formats.
>>
>>>>> +    {
>>>>> +        const EFI_LOAD_OPTION *elo = (const EFI_LOAD_OPTION *)cmdline;
>>>>> +
>>>>> +        /* The absolute minimum the size of the buffer it needs to be */
>>>>> +        size_t size_check = offsetof(EFI_LOAD_OPTION, Description[1]) +
>>>>> +                            elo->FilePathListLength;
>>>>> +
>>>>> +        if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size_check < 
>>>>> cmdsize )
>>>>> +        {
>>>>> +            const CHAR16 *desc = elo->Description;
>>>>> +            size_t desc_length = 0;
>>>>> +
>>>>> +            /* Find Description string length in its possible space */
>>>>> +            while ( desc_length < cmdsize - size_check && *desc++ != 
>>>>> L'\0')
>>>>> +                desc_length += sizeof(*desc);
>>>>> +
>>>>> +            if ( size_check + desc_length < cmdsize )
>>>>> +            {
>>>>> +                *elo_active = true;
>>>>> +                cmdline = (void *)cmdline + size_check + desc_length;
>>>>> +                cmdsize = cmdsize - size_check - desc_length;
>>>>> +            }
>>>>> +        }
>>>>
>>>> I can't help thinking that this is broken: What if you have a structure
>>>> with the LOAD_OPTION_ACTIVE bit clear (leaving aside the fact that
>>>> I'm not sure the meaning of the flag is what you use it for here)?
>>>> That's still not to be taken as a plain command line then.
>>>
>>> Keep in mind that currently everything is being parsed as a plain
>>> command line. So that's the default behavior. All I'm doing in this
>>> patch is falling back on the default behavior if is determined that we
>>> are not dealing with a well-formed EFI_LOAD_OPTION. Doing sanity
>>> checking on arbitrary buffers that may end up being passed here by
>>> buggy shells or buggy firmware or whatnot is beyond the scope of what
>>> I'm looking to accomplish.
>>
>> As per above - this isn't sanity checking. It is a heuristic to tell apart
>> the two possible formats. Without knowing what other formats there
>> might be, there's no way the checking you do is going to be
>> meaningfully more safe than the alternative I'm suggesting. Being
>> given a binary blob, just simply have no way of telling its format
>> without sideband information.
>>
>
> This patch as-is correctly tells the two possible formats apart. I
> tested and Xen boots correctly both from the Shell and from the
> firmware boot menu. I would not like to start addressing hypothetical
> scenarios that I have no reasonable way to test against. If you are
> inclined to do that, that's your call but I'll just leave this patch
> here for now and I hope you would consider merging it.
>
> Tamas

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