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

[Anthony PERARD <anthony.perard@citrix.com>]

On Mon, Jun 27, 2016 at 01:13:43AM -0600, Jan Beulich wrote:
> >>> On 24.06.16 at 19:02, <anthony.perard@citrix.com> wrote:
> > On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote:
> >> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> 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.
> 
> No - once you reach the 4Gb boundary, the compare would continue
> at address zero. That's not what you want.
> > Or I could use the size of name.
> 
> Size of name?

The function get_module_entry() takes an argument called "name", I think
I was proposing to use that, strlen(name).

So, I'm going to add this condition:
(cmdline_paddr + strlen(name) > UINTPTR_MAX)
name is the string we are going to compare cmdline to. I think that
will be enough to do a full check of the module cmdline.


> >> > +        {
> >> > +            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.
> 
> It's even valid when addr+size == UINT_MAX+1 (without value
> wrapping of course), as the last valid byte (i.e. the last one
> hvmloader can address easily) is at 0xffffffff.

I'll do (addr+size-1 > UINTPTR_MAX) which should return true when the
module cross the 4GB boundary.

-- 
Anthony PERARD

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