| Message ID | 0e615fd3-bea9-99a5-fc11-ee0a887b6e80@bitdefender.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/05/17 15:13, Razvan Cojocaru wrote: > On 05/02/17 17:09, Jan Beulich wrote: >>>>> On 02.05.17 at 15:54, <rcojocaru@bitdefender.com> wrote: >>> On 05/02/17 16:48, Jan Beulich wrote: >>>>>>> On 02.05.17 at 15:25, <rcojocaru@bitdefender.com> 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. > So you would prefer something like this? > > diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c > index 78706f5..d4c8d84 100644 > --- a/xen/common/hvm/save.c > +++ b/xen/common/hvm/save.c > @@ -113,6 +113,10 @@ int hvm_save_one(struct domain *d, uint16_t > typecode, uint16_t instance, > const struct hvm_save_descriptor *desc; > > rv = -ENOENT; > + > + if ( !ctxt.cur ) > + goto out; > + > for ( off = 0; off < (ctxt.cur - sizeof(*desc)); off += > desc->length ) > { > desc = (void *)(ctxt.data + off); > @@ -132,6 +136,7 @@ int hvm_save_one(struct domain *d, uint16_t > typecode, uint16_t instance, > } > } > > +out: > xfree(ctxt.data); > return rv; > } For the record, I am -1 for this, because it does not fix the problem when ctxt.cur has a value between 1 and sizeof(*desc). ~Andrew
>>> On 02.05.17 at 16:13, <rcojocaru@bitdefender.com> wrote: > So you would prefer something like this? Not exactly: > --- a/xen/common/hvm/save.c > +++ b/xen/common/hvm/save.c > @@ -113,6 +113,10 @@ int hvm_save_one(struct domain *d, uint16_t > typecode, uint16_t instance, > const struct hvm_save_descriptor *desc; > > rv = -ENOENT; > + > + if ( !ctxt.cur ) if ( ctxt.cur < sizeof(*desc) ) > + goto out; > + > for ( off = 0; off < (ctxt.cur - sizeof(*desc)); off += > desc->length ) > { > desc = (void *)(ctxt.data + off); > @@ -132,6 +136,7 @@ int hvm_save_one(struct domain *d, uint16_t > typecode, uint16_t instance, > } > } > > +out: And this properly indented. Jan
diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c index 78706f5..d4c8d84 100644 --- a/xen/common/hvm/save.c +++ b/xen/common/hvm/save.c @@ -113,6 +113,10 @@ int hvm_save_one(struct domain *d, uint16_t typecode, uint16_t instance, const struct hvm_save_descriptor *desc; rv = -ENOENT; + + if ( !ctxt.cur ) + goto out; + for ( off = 0; off < (ctxt.cur - sizeof(*desc)); off += desc->length )