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

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

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

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