Re: [Xen-devel] [PATCH] xen/hvm: fix hypervisor crash with hvm_save_one()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

On 05/02/17 16:48, Jan Beulich wrote:
>>>> On 02.05.17 at 15:25, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>> hvm_save_cpu_ctxt() does a memset(&ctxt, 0, sizeof(ctxt)), which
>> can lead to ctxt.cur being 0. This can then crash the hypervisor
>> (with FATAL PAGE FAULT) in hvm_save_one() via the
>> "off < (ctxt.cur - sizeof(*desc))" for() test. This has happened
>> in practice with a Linux VM queried around shutdown:
> 
> While a fix is indeed needed here, I can't connect your description
> with the actual crash: The memset() you refer to affects a local
> variable only, which happens to have the same name as a different
> variable in the caller. You also don't make clear at all why this would
> be an issue after guest shutdown was initiated already. Aiui the
> problem is that hvm_save_cpu_ctxt() might find all vCPU-s having
> VPF_down set, in which case it wouldn't put anything into the passed
> in buffer.

That is indeed the case, I've misunderstood the cause of the issue.

>> --- a/xen/common/hvm/save.c
>> +++ b/xen/common/hvm/save.c
>> @@ -113,7 +113,7 @@ int hvm_save_one(struct domain *d, uint16_t typecode, 
>> uint16_t instance,
>>          const struct hvm_save_descriptor *desc;
>>  
>>          rv = -ENOENT;
>> -        for ( off = 0; off < (ctxt.cur - sizeof(*desc)); off += 
>> desc->length )
>> +        for ( off = 0; (off + sizeof(*desc)) < ctxt.cur; off += 
>> desc->length )
>>          {
>>              desc = (void *)(ctxt.data + off);
>>              /* Move past header */
> 
> I don't think this is an appropriate fix. Instead I think the function
> should check whether it got back any data at all, prior to entering
> the loop. Furthermore it might be worth considering to (also)
> refuse doing anything here if the domain's is_dying marker has
> already been set.

hvm_save_one() already checks is_dying:

 77 /* Extract a single instance of a save record, by marshalling all
 78  * records of that type and copying out the one we need. */
 79 int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t
instance,
 80                  XEN_GUEST_HANDLE_64(uint8) handle)
 81 {
 82     int rv = 0;
 83     size_t sz = 0;
 84     struct vcpu *v;
 85     hvm_domain_context_t ctxt = { 0, };
 86
 87     if ( d->is_dying
 88          || typecode > HVM_SAVE_CODE_MAX
 89          || hvm_sr_handlers[typecode].size < sizeof(struct
hvm_save_descriptor)
 90          || hvm_sr_handlers[typecode].save == NULL )
 91         return -EINVAL;

As for checking whether the handler wrote any data, I believe that
Andrew has checked and none of the handlers report when no data is being
passed on.


Thanks,
Razvan

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