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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

> +            {
> +                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. 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? One could
even consider scanning for more than just nul, e.g. any non-blank
character with a numeric value below 0x0020.

Jan


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