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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote:
> >>> 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.

I'll do the full checks.

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

Here, I don't know the size of the cmdline and I don't think calling an
extra strlen() would be usefull. I think that the strcmp() below is going to
be enough for the top bondary check.

Or I could use the size of name.

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

Yes. I'll change the type of module_name and remove the cast here.

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

I think it's valid if addr+size == UINT_MAX. That would means the last
byte of the module would be at 0xFFFFFFFE.

-- 
Anthony PERARD

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