From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] hvm/load: Correct length checks for
 zeroextended records
Date: Fri, 24 Oct 2014 17:03:04 +0100
Message-ID: <544A7838.5060603@citrix.com>
References: <1414061409-569-1-git-send-email-andrew.cooper3@citrix.com>
	<544A8F9A0200007800042083@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <544A8F9A0200007800042083@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: Keir Fraser <keir@xen.org>, Paul Durrant <Paul.Durrant@citrix.com>, Tim Deegan <tim@xen.org>, Xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

On 24/10/14 16:42, Jan Beulich wrote:
>>>> On 23.10.14 at 12:50, <andrew.cooper3@citrix.com> 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@citrix.com>
>> Reviewed-by: Paul Durrant <Paul.Durrant@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 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.

Looks fine, if you are happy to fix this up on commit?

~Andrew