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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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