| Message ID | 5909956E02000078001561D5@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At 00:31 -0600 on 03 May (1493771502), Jan Beulich wrote: > 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? Any of these three patches is fine by me. Cheers, Tim.
On 05/03/17 12:15, Tim Deegan wrote: > At 00:31 -0600 on 03 May (1493771502), Jan Beulich wrote: >> 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? > > Any of these three patches is fine by me. I feel the same. If Andrew prefers this version I'm happy to test it, otherwise re-sending the first patch with the corrected description is the fastest path. Thanks, Razvan
At 10:15 +0100 on 03 May (1493806508), Tim Deegan wrote: > At 00:31 -0600 on 03 May (1493771502), Jan Beulich wrote: > > + 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 ) It occurs to me that as well as underflowing, this test is off by one. It ought to be "off + sizeof(*desc) <= ctxt.cur" to allow for a zero-length record. AFAIK we don't actually have any of those, so it's academic, but we might want to represent the presence of some feature without having any feature-specific state to save. Tim.
>>> On 03.05.17 at 11:21, <tim@xen.org> wrote: > At 10:15 +0100 on 03 May (1493806508), Tim Deegan wrote: >> At 00:31 -0600 on 03 May (1493771502), Jan Beulich wrote: >> > + 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 ) > > It occurs to me that as well as underflowing, this test is off by one. > It ought to be "off + sizeof(*desc) <= ctxt.cur" to allow for a > zero-length record. AFAIK we don't actually have any of those, so > it's academic, but we might want to represent the presence of some > feature without having any feature-specific state to save. Good point; I already have two follow-up patches, one of which I think this adjustment would easily fit into. Jan
On 05/03/17 12:30, Jan Beulich wrote: >>>> On 03.05.17 at 11:21, <tim@xen.org> wrote: >> At 10:15 +0100 on 03 May (1493806508), Tim Deegan wrote: >>> At 00:31 -0600 on 03 May (1493771502), Jan Beulich wrote: >>>> + 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 ) >> >> It occurs to me that as well as underflowing, this test is off by one. >> It ought to be "off + sizeof(*desc) <= ctxt.cur" to allow for a >> zero-length record. AFAIK we don't actually have any of those, so >> it's academic, but we might want to represent the presence of some >> feature without having any feature-specific state to save. > > Good point; I already have two follow-up patches, one of which I > think this adjustment would easily fit into. Should I re-send the original patch with the updated comment then (thus also being able to keep Andrew's Signed-off-by), and if so, is it alright to keep Julien's Release-Acked-by? Or will you use this later patch (presumably with your Signed-off-by), in which case I should test it? Things got rather confusing (apologies for my own part in the confusion). Thanks, Razvan
On 03/05/17 11:44, Razvan Cojocaru wrote: > On 05/03/17 12:30, Jan Beulich wrote: >>>>> On 03.05.17 at 11:21, <tim@xen.org> wrote: >>> At 10:15 +0100 on 03 May (1493806508), Tim Deegan wrote: >>>> At 00:31 -0600 on 03 May (1493771502), Jan Beulich wrote: >>>>> + 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 ) >>> It occurs to me that as well as underflowing, this test is off by one. >>> It ought to be "off + sizeof(*desc) <= ctxt.cur" to allow for a >>> zero-length record. AFAIK we don't actually have any of those, so >>> it's academic, but we might want to represent the presence of some >>> feature without having any feature-specific state to save. >> Good point; I already have two follow-up patches, one of which I >> think this adjustment would easily fit into. > Should I re-send the original patch with the updated comment then (thus > also being able to keep Andrew's Signed-off-by), and if so, is it > alright to keep Julien's Release-Acked-by? > > Or will you use this later patch (presumably with your Signed-off-by), > in which case I should test it? > > Things got rather confusing (apologies for my own part in the confusion). I am not opposed to Jan's alternative, but I think we should make the adjustment to cope with zero length records. At this point, it might be best for Jan to submit a complete patch and for Razvan to double check that it still resolves the issue. ~Andrew
>>> On 03.05.17 at 14:00, <andrew.cooper3@citrix.com> wrote: > On 03/05/17 11:44, Razvan Cojocaru wrote: >> On 05/03/17 12:30, Jan Beulich wrote: >>>>>> On 03.05.17 at 11:21, <tim@xen.org> wrote: >>>> At 10:15 +0100 on 03 May (1493806508), Tim Deegan wrote: >>>>> At 00:31 -0600 on 03 May (1493771502), Jan Beulich wrote: >>>>>> + 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 > ) >>>> It occurs to me that as well as underflowing, this test is off by one. >>>> It ought to be "off + sizeof(*desc) <= ctxt.cur" to allow for a >>>> zero-length record. AFAIK we don't actually have any of those, so >>>> it's academic, but we might want to represent the presence of some >>>> feature without having any feature-specific state to save. >>> Good point; I already have two follow-up patches, one of which I >>> think this adjustment would easily fit into. >> Should I re-send the original patch with the updated comment then (thus >> also being able to keep Andrew's Signed-off-by), and if so, is it >> alright to keep Julien's Release-Acked-by? >> >> Or will you use this later patch (presumably with your Signed-off-by), >> in which case I should test it? >> >> Things got rather confusing (apologies for my own part in the confusion). > > I am not opposed to Jan's alternative, but I think we should make the > adjustment to cope with zero length records. I can certainly move this here from the follow-up patch. > At this point, it might be best for Jan to submit a complete patch and > for Razvan to double check that it still resolves the issue. Let me do that then. Jan
--- 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) )