Re: [Xen-devel] [PATCH v5 08/14] hvmloader: Locate the BIOS blob

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.06.16 at 19:15, <anthony.perard@xxxxxxxxxx> wrote:
> --- a/tools/firmware/hvmloader/hvmloader.c
> +++ b/tools/firmware/hvmloader/hvmloader.c
> @@ -253,10 +253,51 @@ static void acpi_enable_sci(void)
>      BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN));
>  }
>  
> +const struct hvm_modlist_entry *get_module_entry(
> +    const struct hvm_start_info *info,
> +    const char *name)
> +{
> +    const struct hvm_modlist_entry *modlist =
> +        (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr;
> +    unsigned int i;
> +
> +    if ( !modlist || info->modlist_paddr > UINT_MAX)
> +        return NULL;

How about info->modlist_paddr + info->nr_modules * sizeof()?
You check for overflow below, but not here. I think you should
either consistently rely on there being something right below 4Gb
which makes this impossible (and then say so in a comment), or
do full checks everywhere.

> +    for ( i = 0; i < info->nr_modules; i++ )
> +    {
> +        uint32_t module_name = modlist[i].cmdline_paddr;
> +
> +        /* Skip if the module or its cmdline is missing. */
> +        if ( !module_name || !modlist[i].paddr )
> +            continue;
> +
> +        /* Skip if the cmdline can not be read. */
> +        if ( modlist[i].cmdline_paddr > UINT_MAX )
> +            continue;

Similarly here.

> +        if ( !strcmp(name, (char*)module_name) )

Stray cast.

> +        {
> +            if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX ||
> +                 (modlist[i].paddr + modlist[i].size) > UINT_MAX )

I think the last one could be >=.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel