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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan


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