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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Jan 26, 2018 at 5:46 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 23.01.18 at 01:21, <tamas@xxxxxxxxxxxxx> wrote:
>> @@ -88,6 +88,16 @@ typedef struct _EFI_APPLE_PROPERTIES {
>>      EFI_APPLE_PROPERTIES_GETALL GetAll;
>>  } EFI_APPLE_PROPERTIES;
>>
>> +typedef struct _EFI_LOAD_OPTION {
>> +    UINT32 Attributes;
>> +    UINT16 FilePathListLength;
>> +    CHAR16 Description[];
>> +} EFI_LOAD_OPTION;
>> +
>> +#define LOAD_OPTION_ACTIVE              0x00000001
>> +#define LOAD_OPTION_FORCE_RECONNECT     0x00000002
>> +#define LOAD_OPTION_HIDDEN              0x00000008
>
> If you add things we don't really need, please add everything the
> current doc version specifies (i.e. also LOAD_OPTION_CATEGORY*).
> But I'd also be fine if you kept only LOAD_OPTION_ACTIVE.

Declaring only what is used would work fine for me.

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

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

Tamas

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