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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/05/17 14: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.
>
>> --- 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.

This is only one example where no data would be returned.  There are
other examples (e.g. xsave) which can manifest zero data during normal
runtime.

Furthermore, is_dying shouldn't be checked, because it will break the
use of tools such as xen-hvmctx on domains which have been preserved on
crash.  (Although this usecase highlights another rats nest of problems,
when using such tools at the same moment that the toolstack decides to
finally clean up a domain.)

~Andrew

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