* [PATCH] hvm/load: Correct length checks for zeroextended records
@ 2014-10-23 10:50 Andrew Cooper
2014-10-24 15:42 ` Jan Beulich
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Cooper @ 2014-10-23 10:50 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Paul Durrant, Keir Fraser, Jan Beulich, Tim Deegan
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>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
xen/common/hvm/save.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c
index 6c16399..e20b1e6 100644
--- 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)) ||
+ (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);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hvm/load: Correct length checks for zeroextended records
2014-10-23 10:50 [PATCH] hvm/load: Correct length checks for zeroextended records Andrew Cooper
@ 2014-10-24 15:42 ` Jan Beulich
2014-10-24 16:03 ` Andrew Cooper
0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2014-10-24 15:42 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Tim Deegan, Paul Durrant, Keir Fraser, Xen-devel
>>> 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.
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);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hvm/load: Correct length checks for zeroextended records
2014-10-24 15:42 ` Jan Beulich
@ 2014-10-24 16:03 ` Andrew Cooper
0 siblings, 0 replies; 3+ messages in thread
From: Andrew Cooper @ 2014-10-24 16:03 UTC (permalink / raw)
To: Jan Beulich; +Cc: Keir Fraser, Paul Durrant, Tim Deegan, Xen-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-10-24 16:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-23 10:50 [PATCH] hvm/load: Correct length checks for zeroextended records Andrew Cooper
2014-10-24 15:42 ` Jan Beulich
2014-10-24 16:03 ` Andrew Cooper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.