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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.05.17 at 18:11, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 02/05/17 17:02, Tim Deegan wrote:
>> At 18:21 +0300 on 02 May (1493749307), Razvan Cojocaru wrote:
>>> hvm_save_cpu_ctxt() returns success without writing any data into
>>> hvm_domain_context_t when all VCPUs are offline. This can then crash
>>> the hypervisor (with FATAL PAGE FAULT) in hvm_save_one() via the
>>> "off < (ctxt.cur - sizeof(*desc))" for() test, where ctxt.cur remains 0,
>>> causing an underflow which leads the hypervisor to go off the end of the
>>> ctxt buffer.
>> [...]
>>> Reported-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>>> Tested-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
>> I actually preferred the first patch
> 
> As did I.

Hmm, with both of you being of that opinion, I've taken another
look. I think I see now why you think that way (this being data
from an internal producer, overflow/underflow are not a primary
concern), so I'll withdraw my objection to the original patch (i.e.
I agree taking it with the v2 description). However, an alternative
would be

--- unstable.orig/xen/common/hvm/save.c
+++ unstable/xen/common/hvm/save.c
@@ -79,14 +79,15 @@ size_t hvm_save_size(struct domain *d)
 int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance, 
                  XEN_GUEST_HANDLE_64(uint8) handle)
 {
-    int rv = 0;
+    int rv = -ENOENT;
     size_t sz = 0;
     struct vcpu *v;
     hvm_domain_context_t ctxt = { 0, };
+    const struct hvm_save_descriptor *desc;
 
     if ( d->is_dying 
          || typecode > HVM_SAVE_CODE_MAX 
-         || hvm_sr_handlers[typecode].size < sizeof(struct hvm_save_descriptor)
+         || hvm_sr_handlers[typecode].size < sizeof(*desc)
          || hvm_sr_handlers[typecode].save == NULL )
         return -EINVAL;
 
@@ -107,12 +108,10 @@ int hvm_save_one(struct domain *d, uint1
                d->domain_id, typecode);
         rv = -EFAULT;
     }
-    else
+    else if ( ctxt.cur > sizeof(*desc) )
     {
         uint32_t off;
-        const struct hvm_save_descriptor *desc;
 
-        rv = -ENOENT;
         for ( off = 0; off < (ctxt.cur - sizeof(*desc)); off += desc->length )
         {
             desc = (void *)(ctxt.data + off);
@@ -122,7 +121,8 @@ int hvm_save_one(struct domain *d, uint1
             {
                 uint32_t copy_length = desc->length;
 
-                if ( off + copy_length > ctxt.cur )
+                if ( ctxt.cur < copy_length ||
+                     off > ctxt.cur - copy_length )
                     copy_length = ctxt.cur - off;
                 rv = 0;
                 if ( copy_to_guest(handle, ctxt.data + off, copy_length) )

taking care of overflow/underflow (now consistently) as well, plus
avoiding the (imo ugly) goto without making the code harder to
read. Thoughts?

Jan


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