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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.01.19 at 21:43, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

Okay.

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

Right. My comment was to ask that you relax the check accordingly;
I'm not sure whether to interpret your reply that way ...

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

... while this actually makes it sound as if you'd rather want to keep
the too strict check. It's a matter of taste to some degree - to me,
a pointer to an array isn't "dangling" when the array size is known
to be zero. For debugging purposes one may even put in a non-
canonical pointer instead of a NULL one in such cases.

Jan



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