Re: [PATCHv3] xen: Add EFI_LOAD_OPTION support

[Tamas K Lengyel <tamas@tklengyel.com>]

On Mon, Jan 29, 2018 at 2:18 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 26.01.18 at 18:35, <tamas@tklengyel.com> wrote:
>> On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 23.01.18 at 01:21, <tamas@tklengyel.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel