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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Jan 19, 2018 at 2:12 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>> On 04.01.18 at 21:33, <tamas@xxxxxxxxxxxxx> wrote:
>> @@ -375,12 +385,52 @@ 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) )
>> +    {
>> +        /* See include/efi/efiapi.h for more info about the following */
>> +        const EFI_LOAD_OPTION *elo = (const EFI_LOAD_OPTION *)cmdline;
>
> The comment looks to be stale now.
>
>> +        /* The absolute minimum the size of the buffer it needs to be */
>> +        size_t size_check = sizeof(elo->Attributes) +
>> +                            sizeof(elo->FilePathListLength) +
>> +                            elo->FilePathListLength +
>> +                            sizeof(CHAR16);
>
> offsetof(EFI_LOAD_OPTION, Description) + elo->FilePathListLength
> + sizeof(<whatever-the-CHAR16-refers-to>)
>
> I.e. perhaps
>
> offsetof(EFI_LOAD_OPTION, Description[1]) + elo->FilePathListLength
>
>> +        if ( (elo->Attributes & LOAD_OPTION_ACTIVE) && size_check < cmdsize 
>> )
>> +        {
>> +            const CHAR16 *desc = elo->Description;
>> +            const UINT8 *opts = (const UINT8 *)desc;
>
> With gcc's void pointer arithmetic allowing it, some of the code
> would end up easier to read if this was const void *. Also this
> variable should move into the more narrow scope it's used in.
>
>> +            size_t i = 0;
>> +
>> +            /* Find Description string length in its possible space */
>> +            while ( i < cmdsize - size_check && *desc++ != L'\0')
>> +                i += sizeof(CHAR16);
>
> sizeof(*desc) (also elsewhere - please always prefer using the
> type of the actual object over an explicit type).
>
>> +            /* The Description has to end with a NULL char */
>> +            if ( *desc == L'\0' )
>
> How does this work? The loop above has incremented desc already
> past the nul terminator afaict.

Odd, it did work when I tested it. But yes, I'll just check that i is
still within the search space.

>
>> +            {
>> +                UINTN new_cmdsize = cmdsize;
>> +
>> +                opts += i + sizeof(CHAR16) + elo->FilePathListLength;
>
> If you checked that this is within bounds, ...
>
>> +                new_cmdsize -= opts - (UINT8 *)elo;
>> +
>> +                /* Sanity check the new cmdsize to avoid an underflow */
>> +                if ( new_cmdsize < cmdsize )
>
> ... you wouldn't need this check, and you wouldn't need the extra
> new_cmdsize variable at all, likely making more obvious what you're
> doing here.
>
> And then - how about a different heuristic altogether: Current
> code scans the pointed to memory for a nul character, being
> happy if it is found before having exhausted cmdsize.

I'm not sure what you are talking about. The nul char is not at the
end of the EFI_LOAD_OPTION, it is only at the end of the Description
array. And we have to scan for it because the length of the
Description array is not recorded anywhere else. So we scan for the
nul char in the part of the buffer where it would be warranted by the
EFI spec, then compute the location of the actual option buffer based
on that checking that it is within bounds of the buffer.

> Why not
> simply scan for the first nul and conclude it's an EFI_LOAD_OPTION
> structure if that nul isn't right at the end of the buffer?

I don't follow, how would that be more robust then checking that NULL
is in the right space within the EFI_LOAD_OPTION according to spec?

> One could
> even consider scanning for more than just nul, e.g. any non-blank
> character with a numeric value below 0x0020.

Perhaps, but I think that's an overkill. Again, what are the chances
that a non-EFI loader will pass a string buffer that happens to also
properly parse as an EFI_LOAD_OPTION?

Tamas

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