Re: [Xen-devel] [PATCH] hvm/load: Correct length checks for zeroextended records

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.10.14 at 12:50, <andrew.cooper3@xxxxxxxxxx> wrote:
> In the case that Xen is attempting to load a zeroextended HVM record where the
> difference needing extending would overflow the data blob, 
> _hvm_check_entry()
> will incorrectly fail before working out that it would have been safe.
> 
> The "len + sizeof(*d)" check is wrong.  Consider zeroextending a 16 byte
> record into a 32 byte structure.  "32 + hdr" will fail the overall context
> length check even though the pre-extended record in the stream is 16 bytes.
> 
> The first condition is reduced to just a length check for hvm save header,
> while the second condition is extended to include a check that the record in
> the stream not exceeding the stream length.
> 
> The error messages are extended to include further useful information.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Paul Durrant <Paul.Durrant@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit with a comment:

> --- a/xen/common/hvm/save.c
> +++ b/xen/common/hvm/save.c
> @@ -292,19 +292,22 @@ int _hvm_check_entry(struct hvm_domain_context *h,
>  {
>      struct hvm_save_descriptor *d 
>          = (struct hvm_save_descriptor *)&h->data[h->cur];
> -    if ( len + sizeof (*d) > h->size - h->cur)
> +    if ( sizeof(*d) > h->size - h->cur)
>      {
>          printk(XENLOG_G_WARNING
> -               "HVM restore: not enough data left to read %u bytes "
> -               "for type %u\n", len, type);
> +               "HVM restore: not enough data left to read %zu bytes "
> +               "for type %u header\n", sizeof(*d), type);
>          return -1;
> -    }    
> +    }
>      if ( (type != d->typecode) || (len < d->length) ||
> -         (strict_length && (len != d->length)) )
> +         (strict_length && (len != d->length)) ||

If this is already being overhauled, I'd really like to at once switch to

     if ( (type != d->typecode) ||
         (strict_length ? (len != d->length) : (len < d->length)) ||

to make more obvious what is really being checked here. Definitely
no reason to re-submit though.

Jan

> +         (d->length > (h->size - h->cur - sizeof(*d))) )
>      {
>          printk(XENLOG_G_WARNING
> -               "HVM restore mismatch: expected type %u length %u, "
> -               "saw type %u length %u\n", type, len, d->typecode, d->length);
> +               "HVM restore mismatch: expected %s type %u length %u, "
> +               "saw type %u length %u.  %zu bytes remaining\n",
> +               strict_length ? "strict" : "zeroextended", type, len,
> +               d->typecode, d->length, h->size - h->cur - sizeof(*d));
>          return -1;
>      }
>      h->cur += sizeof(*d);



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