Re: [Xen-devel] [PATCH 2/3] x86/pvh: Fixes to convert_pvh_info()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 23/01/2019 11:35, Jan Beulich wrote:
>>>> On 21.01.19 at 16:37, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/guest/pvh-boot.c
>> +++ b/xen/arch/x86/guest/pvh-boot.c
>> @@ -38,12 +38,20 @@ static const char *__initdata pvh_loader = "PVH 
>> Directboot";
>>  static void __init convert_pvh_info(multiboot_info_t **mbi,
>>                                      module_t **mod)
>>  {
>> -    const struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
>> +    struct hvm_start_info *pvh_info = __va(pvh_start_info_pa);
>>      const struct hvm_modlist_entry *entry;
>>      unsigned int i;
>>  
>>      if ( pvh_info->magic != XEN_HVM_START_MAGIC_VALUE )
>> -        panic("Magic value is wrong: %x\n", pvh_info->magic);
>> +        panic("PVH magic value is wrong: %x\n", pvh_info->magic);
>> +
>> +    /* Check consistency between the modlist and number of modules. */
>> +    if ( (pvh_info->modlist_paddr == 0) != (pvh_info->nr_modules == 0) )
>> +    {
>> +        printk("PVH module mismatch: pa %08"PRIx64", nr %u - Ignoring\n",
>> +               pvh_info->modlist_paddr, pvh_info->nr_modules);
>> +        pvh_info->modlist_paddr = pvh_info->nr_modules = 0;
>> +    }
> While we don't consume memmap_{paddr,entries} (yet), wouldn't
> it make sense to also check those for similar consistency?

Plausibly, but as you note, its not like we use any of that yet.  Also,
it needs an ABI version check, so I'm not going to complicated this
patch with speculative work.

>
> Furthermore I'm not convinced the check above is correct: I don't
> see anything wrong with a random modlist_paddr as long as
> nr_modules is zero.

The problem case is the opposite way around - when nr_modules is nonzero
and paddr is 0.

Several of the loops in Xen will really go wrong if then encounter such
a malformed entry.

> In particular it is not uncommon for placement
> implementations to assign the next sequential address to the next
> item to process before looking at or iterating over the number of
> associated entries.

I'd put that firmly in the class of "buggy firmware".  Leaving dangling
pointers is never a good idea, even if it isn't strictly speaking in
violation of the protocol in question.

~Andrew

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