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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.05.17 at 15:54, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
> On 05/02/17 16:48, Jan Beulich wrote:
>>>>> On 02.05.17 at 15:25, <rcojocaru@xxxxxxxxxxxxxxx> wrote:
>>> --- 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;

Hmm, interesting. The timing window to see is_dying clear here,
bit no vCPU-s left there should be pretty small, so I wonder how
you've managed to hit it. But anyway ...

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

... that's not what I've read out of his replies. I don't think the
handlers need to report anything special. It is the caller which
should check whether, despite having got back "success" there's
no data in the buffer.

Jan


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