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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.06.16 at 19:02, <anthony.perard@xxxxxxxxxx> wrote:
> 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.

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?

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

Jan


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