Discussion:
[oss-security] Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs
(too old to reply)
Steven M. Christey
2012-12-13 22:03:23 UTC
Permalink
All,

This advisory required two different CVE IDs - not one - because the
stack-based buffer overflow was fixed in a different version than the
other issues. CVE assigns different IDs when bugs are not present in the
same exact set of versions.

CVE-2012-5511 - use this, but only for the stack-based buffer overflow
that was fixed in 4.2.

CVE-2012-6333 - new ID for the other "large input" validation issues that
lead to the physical CPU hang, which were NOT fixed in 4.2.


- Steve
Matt Wilson
2013-01-12 15:35:31 UTC
Permalink
On Mon, Dec 03, 2012 at 05:51:44PM +0000, Xen.org security team wrote:
[...]
ISSUE DESCRIPTION
=================
Several HVM control operations do not check the size of their inputs
and can tie up a physical CPU for extended periods of time.
In addition dirty video RAM tracking involves clearing the bitmap
provided by the domain controlling the guest (e.g. dom0 or a
stubdom). If the size of that bitmap is overly large, an intermediate
variable on the hypervisor stack may overflow that stack.
Part of the xsa27-4.1.patch, quoted below, might have a bug.
x86/paging: Don't allocate user-controlled amounts of stack memory.
This is XSA-27 / CVE-2012-5511.
v2: Provide definition of GB to fix x86-32 compile.
[...]
diff -r 5639047d6c9f xen/arch/x86/mm/paging.c
--- a/xen/arch/x86/mm/paging.c Mon Nov 19 09:43:48 2012 +0100
+++ b/xen/arch/x86/mm/paging.c Mon Nov 19 16:00:33 2012 +0000
@@ -529,13 +529,18 @@ int paging_log_dirty_range(struct domain
if ( !d->arch.paging.log_dirty.fault_count &&
!d->arch.paging.log_dirty.dirty_count ) {
- int size = (nr + BITS_PER_LONG - 1) / BITS_PER_LONG;
- unsigned long zeroes[size];
- memset(zeroes, 0x00, size * BYTES_PER_LONG);
+ static uint8_t zeroes[PAGE_SIZE];
+ int off, size;
+
+ size = ((nr + BITS_PER_LONG - 1) / BITS_PER_LONG) * sizeof (long);
rv = 0;
- if ( copy_to_guest_offset(dirty_bitmap, 0, (uint8_t *) zeroes,
- size * BYTES_PER_LONG) != 0 )
- rv = -EFAULT;
+ for ( off = 0; !rv && off < size; off += sizeof(zeroes) )
+ {
+ int todo = min(size - off, (int) PAGE_SIZE);
+ if ( copy_to_guest_offset(dirty_bitmap, off, zeroes, todo) )
+ rv = -EFAULT;
+ off += todo;
off is incremented twice, once as part of the for loop and once
inside. Was that intended?

Credit to Steven Noonan for pointing this out.

Matt
+ }
goto out;
}
d->arch.paging.log_dirty.fault_count = 0;
Ian Campbell
2013-01-14 10:23:34 UTC
Permalink
Post by Matt Wilson
diff -r 5639047d6c9f xen/arch/x86/mm/paging.c
--- a/xen/arch/x86/mm/paging.c Mon Nov 19 09:43:48 2012 +0100
+++ b/xen/arch/x86/mm/paging.c Mon Nov 19 16:00:33 2012 +0000
@@ -529,13 +529,18 @@ int paging_log_dirty_range(struct domain
if ( !d->arch.paging.log_dirty.fault_count &&
!d->arch.paging.log_dirty.dirty_count ) {
- int size = (nr + BITS_PER_LONG - 1) / BITS_PER_LONG;
- unsigned long zeroes[size];
- memset(zeroes, 0x00, size * BYTES_PER_LONG);
+ static uint8_t zeroes[PAGE_SIZE];
+ int off, size;
+
+ size = ((nr + BITS_PER_LONG - 1) / BITS_PER_LONG) * sizeof (long);
rv = 0;
- if ( copy_to_guest_offset(dirty_bitmap, 0, (uint8_t *) zeroes,
- size * BYTES_PER_LONG) != 0 )
- rv = -EFAULT;
+ for ( off = 0; !rv && off < size; off += sizeof(zeroes) )
+ {
+ int todo = min(size - off, (int) PAGE_SIZE);
+ if ( copy_to_guest_offset(dirty_bitmap, off, zeroes, todo) )
+ rv = -EFAULT;
+ off += todo;
off is incremented twice, once as part of the for loop and once
inside. Was that intended?
It certainly does seem wrong (or too clever for me).

I think either could correctly be removed but the more logical one would
be the one in the for loop, I think, since the one inside the body is
more accurate (although it only matters for the final iteration and
either would cause the loop to exit).

Ian.
Jan Beulich
2013-01-14 10:52:19 UTC
Permalink
Post by Ian Campbell
Post by Matt Wilson
diff -r 5639047d6c9f xen/arch/x86/mm/paging.c
--- a/xen/arch/x86/mm/paging.c Mon Nov 19 09:43:48 2012 +0100
+++ b/xen/arch/x86/mm/paging.c Mon Nov 19 16:00:33 2012 +0000
@@ -529,13 +529,18 @@ int paging_log_dirty_range(struct domain
if ( !d->arch.paging.log_dirty.fault_count &&
!d->arch.paging.log_dirty.dirty_count ) {
- int size = (nr + BITS_PER_LONG - 1) / BITS_PER_LONG;
- unsigned long zeroes[size];
- memset(zeroes, 0x00, size * BYTES_PER_LONG);
+ static uint8_t zeroes[PAGE_SIZE];
+ int off, size;
+
+ size = ((nr + BITS_PER_LONG - 1) / BITS_PER_LONG) * sizeof (long);
rv = 0;
- if ( copy_to_guest_offset(dirty_bitmap, 0, (uint8_t *) zeroes,
- size * BYTES_PER_LONG) != 0 )
- rv = -EFAULT;
+ for ( off = 0; !rv && off < size; off += sizeof(zeroes) )
+ {
+ int todo = min(size - off, (int) PAGE_SIZE);
+ if ( copy_to_guest_offset(dirty_bitmap, off, zeroes, todo) )
+ rv = -EFAULT;
+ off += todo;
off is incremented twice, once as part of the for loop and once
inside. Was that intended?
It certainly does seem wrong (or too clever for me).
I think either could correctly be removed but the more logical one would
be the one in the for loop, I think, since the one inside the body is
more accurate (although it only matters for the final iteration and
either would cause the loop to exit).
I agree, but since it was Tim who had put this on-off together
(as a smaller replacement to the fix that went into 4.2 before
its release), I'd leave this to him.

Jan
Tim Deegan
2013-01-14 12:19:24 UTC
Permalink
Post by Jan Beulich
Post by Ian Campbell
Post by Matt Wilson
diff -r 5639047d6c9f xen/arch/x86/mm/paging.c
--- a/xen/arch/x86/mm/paging.c Mon Nov 19 09:43:48 2012 +0100
+++ b/xen/arch/x86/mm/paging.c Mon Nov 19 16:00:33 2012 +0000
@@ -529,13 +529,18 @@ int paging_log_dirty_range(struct domain
if ( !d->arch.paging.log_dirty.fault_count &&
!d->arch.paging.log_dirty.dirty_count ) {
- int size = (nr + BITS_PER_LONG - 1) / BITS_PER_LONG;
- unsigned long zeroes[size];
- memset(zeroes, 0x00, size * BYTES_PER_LONG);
+ static uint8_t zeroes[PAGE_SIZE];
+ int off, size;
+
+ size = ((nr + BITS_PER_LONG - 1) / BITS_PER_LONG) * sizeof (long);
rv = 0;
- if ( copy_to_guest_offset(dirty_bitmap, 0, (uint8_t *) zeroes,
- size * BYTES_PER_LONG) != 0 )
- rv = -EFAULT;
+ for ( off = 0; !rv && off < size; off += sizeof(zeroes) )
+ {
+ int todo = min(size - off, (int) PAGE_SIZE);
+ if ( copy_to_guest_offset(dirty_bitmap, off, zeroes, todo) )
+ rv = -EFAULT;
+ off += todo;
off is incremented twice, once as part of the for loop and once
inside. Was that intended?
It certainly does seem wrong (or too clever for me).
I think either could correctly be removed but the more logical one would
be the one in the for loop, I think, since the one inside the body is
more accurate (although it only matters for the final iteration and
either would cause the loop to exit).
I agree, but since it was Tim who had put this on-off together
(as a smaller replacement to the fix that went into 4.2 before
its release), I'd leave this to him.
Yeah, the increment in the loop header shouldn't be there.

I can't get to xenbits for an up-to-date tree right now but I'll make a
patch once I can.

Tim.
Tim Deegan
2013-01-17 11:30:12 UTC
Permalink
# HG changeset patch
# User Tim Deegan <***@xen.org>
# Date 1358421452 0
# Node ID 04368044ca5fb9800bfdacf14e883d39cad5c8a6
# Parent 8fe0e86c2ac27e22121aa9c70ddf5eacbb3051d0
x86/mm: Fix loop increment in paging_log_dirty_range()

In 23417:53ef1f35a0f8 (the fix for XSA-27 / CVE-2012-5511), the
loop variable gets incremented twice, so the loop only clears every
second page of the bitmap. This might cause the tools to think that
pages are dirty when they are not.

Reported-by: Steven Noonan <***@amazon.com>
Reported-by: Matt Wilson <***@amazon.com>
Signed-off-by: Tim Deegan <***@xen.org>

diff -r 8fe0e86c2ac2 -r 04368044ca5f xen/arch/x86/mm/paging.c
--- a/xen/arch/x86/mm/paging.c Wed Jan 16 14:15:12 2013 +0000
+++ b/xen/arch/x86/mm/paging.c Thu Jan 17 11:17:32 2013 +0000
@@ -534,7 +534,8 @@ int paging_log_dirty_range(struct domain

size = ((nr + BITS_PER_LONG - 1) / BITS_PER_LONG) * sizeof (long);
rv = 0;
- for ( off = 0; !rv && off < size; off += sizeof zeroes )
+ off = 0;
+ while ( !rv && off < size )
{
int todo = min(size - off, (int) PAGE_SIZE);
if ( copy_to_guest_offset(dirty_bitmap, off, zeroes, todo) )
Ian Campbell
2013-01-17 11:39:52 UTC
Permalink
Post by Tim Deegan
# HG changeset patch
# Date 1358421452 0
# Node ID 04368044ca5fb9800bfdacf14e883d39cad5c8a6
# Parent 8fe0e86c2ac27e22121aa9c70ddf5eacbb3051d0
x86/mm: Fix loop increment in paging_log_dirty_range()
In 23417:53ef1f35a0f8 (the fix for XSA-27 / CVE-2012-5511), the
loop variable gets incremented twice, so the loop only clears every
second page of the bitmap. This might cause the tools to think that
pages are dirty when they are not.
diff -r 8fe0e86c2ac2 -r 04368044ca5f xen/arch/x86/mm/paging.c
--- a/xen/arch/x86/mm/paging.c Wed Jan 16 14:15:12 2013 +0000
+++ b/xen/arch/x86/mm/paging.c Thu Jan 17 11:17:32 2013 +0000
@@ -534,7 +534,8 @@ int paging_log_dirty_range(struct domain
size = ((nr + BITS_PER_LONG - 1) / BITS_PER_LONG) * sizeof (long);
rv = 0;
- for ( off = 0; !rv && off < size; off += sizeof zeroes )
+ off = 0;
+ while ( !rv && off < size )
{
int todo = min(size - off, (int) PAGE_SIZE);
if ( copy_to_guest_offset(dirty_bitmap, off, zeroes, todo) )
_______________________________________________
Xen-devel mailing list
http://lists.xen.org/xen-devel
Loading...