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

[Tim Deegan <tim@xxxxxxx>]

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.

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