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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

> +    {
> +        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. As I thought
I had expressed earlier, you really _only_ want the (improved) earlier
check. If it's a load option structure, you'll find a nul at the end of the
description, which lives earlier than the indicated overall size, since
OptionalData is specified to be on non-zero length (or else a NULL
pointer is supposedly passed to us; maybe they actually mean a nul
character, but then its size would again be non-zero).

Jan


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