Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs

Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs

[Xen.org security team]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

	     Xen Security Advisory CVE-2012-5511 / XSA-27
                           version 4

   several HVM operations do not validate the range of their inputs

UPDATES IN VERSION 4
====================

Public release.

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.

IMPACT
======

A malicious guest administrator can cause Xen to become unresponsive
or to crash leading in either case to a Denial of Service.

VULNERABLE SYSTEMS
==================

All Xen versions from 3.4 onwards are vulnerable.

However Xen 4.2 and unstable are not vulnerable to the stack
overflow. Systems running either of these are not vulnerable to the
crash.

Version 3.4, 4.0 and 4.1 are vulnerable to both the stack overflow and
the physical CPU hang.

The vulnerability is only exposed to HVM guests.

MITIGATION
==========

Running only PV guests will avoid this vulnerability.

RESOLUTION
==========

Applying the appropriate attached patch resolves this issue.

xsa27-4.1.patch             Xen 4.1.x
xsa27-4.2.patch             Xen 4.2.x
xsa27-4.unstable.patch      xen-unstable


$ sha256sum xsa27*.patch
7443da829a7b2dd4b5e0b8db97a8b569e7c10d908ee7c34fa60bc2ddd781be57  xsa27-4.1.patch
462eae827944d1d337a6ebf13a36ea952d7fb76b993b9c29946e1d9cfb5ea2a3  xsa27-4.2.patch
fcb07c6bd78a0d9513a68e2eb3bf0c21ef4d8ff0e6ebf6fdce04a3170303cab6  xsa27-unstable.patch
$
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJQvOJ2AAoJEIP+FMlX6CvZzqwIAJwIUGfXDA0KvJ/zZWAJm49Q
c5Sn5xK1wZdGdJTlCqAGZSMOmaUP6tofqEWanb6nOg2vRAk7HlDz1JbUw5P8E3H9
mTT9Ro8rOhAIhgD0joT4i2XE77OTuLF85JK0M0fn2XPdUNFraChYUGthXj9+irlc
FOhrLnXBlo34h7V7nY9XGIKAwcYUQnR7RcPasKOCO1OGEYofWKJOSKR9wrIhXiMN
Q2svs4J1+PxNdKpErS+mMwEbnYHBcmxxEZXWktB9plzSqf5FMP4yQ3C5wTu/zrYH
nu8Jj2JNV3NTnZgcviUBysTR+1s+JgVjLU3gtxebh2caqjSKyenPU2yYna5rlfY=
=tfAP
-----END PGP SIGNATURE-----

[-- Attachment #2: xsa27-4.1.patch --]
[-- Type: application/octet-stream, Size: 5821 bytes --]

hvm: Limit the size of large HVM op batches

Doing large p2m updates for HVMOP_track_dirty_vram without preemption
ties up the physical processor. Integrating preemption into the p2m
updates is hard so simply limit to 1GB which is sufficient for a 15000
* 15000 * 32bpp framebuffer.

For HVMOP_modified_memory and HVMOP_set_mem_type preemptible add the
necessary machinery to handle preemption.

This is CVE-2012-5511 / XSA-27.

Signed-off-by: Tim Deegan <tim@xen.org>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

x86/paging: Don't allocate user-controlled amounts of stack memory.

This is XSA-27 / CVE-2012-5511.

Signed-off-by: Tim Deegan <tim@xen.org>
Acked-by: Jan Beulich <jbeulich@suse.com>
v2: Provide definition of GB to fix x86-32 compile.

Signed-off-by: Jan Beulich <JBeulich@suse.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>


diff -r 5639047d6c9f xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Mon Nov 19 09:43:48 2012 +0100
+++ b/xen/arch/x86/hvm/hvm.c	Mon Nov 19 16:00:33 2012 +0000
@@ -3471,6 +3471,9 @@ long do_hvm_op(unsigned long op, XEN_GUE
         if ( !is_hvm_domain(d) )
             goto param_fail2;
 
+        if ( a.nr > GB(1) >> PAGE_SHIFT )
+            goto param_fail2;
+
         rc = xsm_hvm_param(d, op);
         if ( rc )
             goto param_fail2;
@@ -3498,7 +3501,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
         struct xen_hvm_modified_memory a;
         struct domain *d;
         struct p2m_domain *p2m;
-        unsigned long pfn;
 
         if ( copy_from_guest(&a, arg, 1) )
             return -EFAULT;
@@ -3526,8 +3528,9 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail3;
 
         p2m = p2m_get_hostp2m(d);
-        for ( pfn = a.first_pfn; pfn < a.first_pfn + a.nr; pfn++ )
+        while ( a.nr > 0 )
         {
+            unsigned long pfn = a.first_pfn;
             p2m_type_t t;
             mfn_t mfn = gfn_to_mfn(p2m, pfn, &t);
             if ( p2m_is_paging(t) )
@@ -3548,6 +3551,19 @@ long do_hvm_op(unsigned long op, XEN_GUE
                 /* don't take a long time and don't die either */
                 sh_remove_shadows(d->vcpu[0], mfn, 1, 0);
             }
+
+            a.first_pfn++;
+            a.nr--;
+
+            /* Check for continuation if it's not the last interation */
+            if ( a.nr > 0 && hypercall_preempt_check() )
+            {
+                if ( copy_to_guest(arg, &a, 1) )
+                    rc = -EFAULT;
+                else
+                    rc = -EAGAIN;
+                break;
+            }
         }
 
     param_fail3:
@@ -3595,7 +3611,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
         struct xen_hvm_set_mem_type a;
         struct domain *d;
         struct p2m_domain *p2m;
-        unsigned long pfn;
         
         /* Interface types to internal p2m types */
         p2m_type_t memtype[] = {
@@ -3625,8 +3640,9 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail4;
 
         p2m = p2m_get_hostp2m(d);
-        for ( pfn = a.first_pfn; pfn < a.first_pfn + a.nr; pfn++ )
+        while ( a.nr > 0 )
         {
+            unsigned long pfn = a.first_pfn;
             p2m_type_t t;
             p2m_type_t nt;
             mfn_t mfn;
@@ -3662,6 +3678,19 @@ long do_hvm_op(unsigned long op, XEN_GUE
                     goto param_fail4;
                 }
             }
+
+            a.first_pfn++;
+            a.nr--;
+
+            /* Check for continuation if it's not the last interation */
+            if ( a.nr > 0 && hypercall_preempt_check() )
+            {
+                if ( copy_to_guest(arg, &a, 1) )
+                    rc = -EFAULT;
+                else
+                    rc = -EAGAIN;
+                goto param_fail4;
+            }
         }
 
         rc = 0;
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;
+        }
         goto out;
     }
     d->arch.paging.log_dirty.fault_count = 0;
diff -r 5639047d6c9f xen/include/asm-x86/config.h
--- a/xen/include/asm-x86/config.h	Mon Nov 19 09:43:48 2012 +0100
+++ b/xen/include/asm-x86/config.h	Mon Nov 19 16:00:33 2012 +0000
@@ -108,6 +108,9 @@ extern unsigned int trampoline_xen_phys_
 extern unsigned char trampoline_cpu_started;
 extern char wakeup_start[];
 extern unsigned int video_mode, video_flags;
+
+#define GB(_gb) (_gb ## UL << 30)
+
 #endif
 
 #define asmlinkage
@@ -123,7 +126,6 @@ extern unsigned int video_mode, video_fl
 #define PML4_ADDR(_slot)                             \
     ((((_slot ## UL) >> 8) * 0xffff000000000000UL) | \
      (_slot ## UL << PML4_ENTRY_BITS))
-#define GB(_gb) (_gb ## UL << 30)
 #else
 #define PML4_ENTRY_BYTES (1 << PML4_ENTRY_BITS)
 #define PML4_ADDR(_slot)                             \

[-- Attachment #3: xsa27-4.2.patch --]
[-- Type: application/octet-stream, Size: 4441 bytes --]

hvm: Limit the size of large HVM op batches

Doing large p2m updates for HVMOP_track_dirty_vram without preemption
ties up the physical processor. Integrating preemption into the p2m
updates is hard so simply limit to 1GB which is sufficient for a 15000
* 15000 * 32bpp framebuffer.

For HVMOP_modified_memory and HVMOP_set_mem_type preemptible add the
necessary machinery to handle preemption.

This is CVE-2012-5511 / XSA-27.

Signed-off-by: Tim Deegan <tim@xen.org>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

v2: Provide definition of GB to fix x86-32 compile.

Signed-off-by: Jan Beulich <JBeulich@suse.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>


diff -r 7c4d806b3753 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Fri Nov 16 15:56:14 2012 +0000
+++ b/xen/arch/x86/hvm/hvm.c	Mon Nov 19 14:42:10 2012 +0000
@@ -3969,6 +3969,9 @@ long do_hvm_op(unsigned long op, XEN_GUE
         if ( !is_hvm_domain(d) )
             goto param_fail2;
 
+        if ( a.nr > GB(1) >> PAGE_SHIFT )
+            goto param_fail2;
+
         rc = xsm_hvm_param(d, op);
         if ( rc )
             goto param_fail2;
@@ -3995,7 +3998,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
     {
         struct xen_hvm_modified_memory a;
         struct domain *d;
-        unsigned long pfn;
 
         if ( copy_from_guest(&a, arg, 1) )
             return -EFAULT;
@@ -4022,9 +4024,11 @@ long do_hvm_op(unsigned long op, XEN_GUE
         if ( !paging_mode_log_dirty(d) )
             goto param_fail3;
 
-        for ( pfn = a.first_pfn; pfn < a.first_pfn + a.nr; pfn++ )
+        while ( a.nr > 0 )
         {
+            unsigned long pfn = a.first_pfn;
             struct page_info *page;
+
             page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
             if ( page )
             {
@@ -4034,6 +4038,19 @@ long do_hvm_op(unsigned long op, XEN_GUE
                 sh_remove_shadows(d->vcpu[0], _mfn(page_to_mfn(page)), 1, 0);
                 put_page(page);
             }
+
+            a.first_pfn++;
+            a.nr--;
+
+            /* Check for continuation if it's not the last interation */
+            if ( a.nr > 0 && hypercall_preempt_check() )
+            {
+                if ( copy_to_guest(arg, &a, 1) )
+                    rc = -EFAULT;
+                else
+                    rc = -EAGAIN;
+                break;
+            }
         }
 
     param_fail3:
@@ -4089,7 +4106,6 @@ long do_hvm_op(unsigned long op, XEN_GUE
     {
         struct xen_hvm_set_mem_type a;
         struct domain *d;
-        unsigned long pfn;
         
         /* Interface types to internal p2m types */
         p2m_type_t memtype[] = {
@@ -4122,8 +4138,9 @@ long do_hvm_op(unsigned long op, XEN_GUE
         if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
             goto param_fail4;
 
-        for ( pfn = a.first_pfn; pfn < a.first_pfn + a.nr; pfn++ )
+        while ( a.nr )
         {
+            unsigned long pfn = a.first_pfn;
             p2m_type_t t;
             p2m_type_t nt;
             mfn_t mfn;
@@ -4163,6 +4180,19 @@ long do_hvm_op(unsigned long op, XEN_GUE
                 }
             }
             put_gfn(d, pfn);
+
+            a.first_pfn++;
+            a.nr--;
+
+            /* Check for continuation if it's not the last interation */
+            if ( a.nr > 0 && hypercall_preempt_check() )
+            {
+                if ( copy_to_guest(arg, &a, 1) )
+                    rc = -EFAULT;
+                else
+                    rc = -EAGAIN;
+                goto param_fail4;
+            }
         }
 
         rc = 0;
diff -r 7c4d806b3753 xen/include/asm-x86/config.h
--- a/xen/include/asm-x86/config.h	Fri Nov 16 15:56:14 2012 +0000
+++ b/xen/include/asm-x86/config.h	Mon Nov 19 14:42:10 2012 +0000
@@ -119,6 +119,9 @@ extern char wakeup_start[];
 extern unsigned int video_mode, video_flags;
 extern unsigned short boot_edid_caps;
 extern unsigned char boot_edid_info[128];
+
+#define GB(_gb) (_gb ## UL << 30)
+
 #endif
 
 #define asmlinkage
@@ -134,7 +137,6 @@ extern unsigned char boot_edid_info[128]
 #define PML4_ADDR(_slot)                             \
     ((((_slot ## UL) >> 8) * 0xffff000000000000UL) | \
      (_slot ## UL << PML4_ENTRY_BITS))
-#define GB(_gb) (_gb ## UL << 30)
 #else
 #define PML4_ENTRY_BYTES (1 << PML4_ENTRY_BITS)
 #define PML4_ADDR(_slot)                             \

[-- Attachment #4: xsa27-unstable.patch --]
[-- Type: application/octet-stream, Size: 3669 bytes --]

hvm: Limit the size of large HVM op batches

Doing large p2m updates for HVMOP_track_dirty_vram without preemption
ties up the physical processor. Integrating preemption into the p2m
updates is hard so simply limit to 1GB which is sufficient for a 15000
* 15000 * 32bpp framebuffer.

For HVMOP_modified_memory and HVMOP_set_mem_type preemptible add the
necessary machinery to handle preemption.

This is CVE-2012-5511 / XSA-27.

Signed-off-by: Tim Deegan <tim@xen.org>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 34da2f5..2d46d98 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3984,6 +3984,9 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( !is_hvm_domain(d) )
             goto param_fail2;
 
+        if ( a.nr > GB(1) >> PAGE_SHIFT )
+            goto param_fail2;
+
         rc = xsm_hvm_param(d, op);
         if ( rc )
             goto param_fail2;
@@ -4010,7 +4013,6 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
     {
         struct xen_hvm_modified_memory a;
         struct domain *d;
-        unsigned long pfn;
 
         if ( copy_from_guest(&a, arg, 1) )
             return -EFAULT;
@@ -4037,9 +4039,11 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( !paging_mode_log_dirty(d) )
             goto param_fail3;
 
-        for ( pfn = a.first_pfn; pfn < a.first_pfn + a.nr; pfn++ )
+        while ( a.nr > 0 )
         {
+            unsigned long pfn = a.first_pfn;
             struct page_info *page;
+
             page = get_page_from_gfn(d, pfn, NULL, P2M_UNSHARE);
             if ( page )
             {
@@ -4049,6 +4053,19 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
                 sh_remove_shadows(d->vcpu[0], _mfn(page_to_mfn(page)), 1, 0);
                 put_page(page);
             }
+
+            a.first_pfn++;
+            a.nr--;
+
+            /* Check for continuation if it's not the last interation */
+            if ( a.nr > 0 && hypercall_preempt_check() )
+            {
+                if ( copy_to_guest(arg, &a, 1) )
+                    rc = -EFAULT;
+                else
+                    rc = -EAGAIN;
+                break;
+            }
         }
 
     param_fail3:
@@ -4104,7 +4121,6 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
     {
         struct xen_hvm_set_mem_type a;
         struct domain *d;
-        unsigned long pfn;
         
         /* Interface types to internal p2m types */
         p2m_type_t memtype[] = {
@@ -4137,8 +4153,9 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( a.hvmmem_type >= ARRAY_SIZE(memtype) )
             goto param_fail4;
 
-        for ( pfn = a.first_pfn; pfn < a.first_pfn + a.nr; pfn++ )
+        while ( a.nr )
         {
+            unsigned long pfn = a.first_pfn;
             p2m_type_t t;
             p2m_type_t nt;
             mfn_t mfn;
@@ -4178,6 +4195,19 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
                 }
             }
             put_gfn(d, pfn);
+
+            a.first_pfn++;
+            a.nr--;
+
+            /* Check for continuation if it's not the last interation */
+            if ( a.nr > 0 && hypercall_preempt_check() )
+            {
+                if ( copy_to_guest(arg, &a, 1) )
+                    rc = -EFAULT;
+                else
+                    rc = -EAGAIN;
+                goto param_fail4;
+            }
         }
 
         rc = 0;

[-- Attachment #5: Type: text/plain, Size: 126 bytes --]

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

Re: [oss-security] Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs

[Steven M. Christey]

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

Re: Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs

[Matt Wilson]

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.
> 
> Signed-off-by: Tim Deegan <tim@xen.org>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> v2: Provide definition of GB to fix x86-32 compile.
> 
> Signed-off-by: Jan Beulich <JBeulich@suse.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
[...]
> 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;

Re: Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs

[Ian Campbell]

On Sat, 2013-01-12 at 15:35 +0000, Matt Wilson wrote:
> > 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.

Re: Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs

[Jan Beulich]

>>> On 14.01.13 at 11:23, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Sat, 2013-01-12 at 15:35 +0000, Matt Wilson wrote:
>> > 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

Re: Xen Security Advisory 27 (CVE-2012-5511) - several HVM operations do not validate the range of their inputs

[Tim Deegan]

At 10:52 +0000 on 14 Jan (1358160739), Jan Beulich wrote:
> >>> On 14.01.13 at 11:23, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Sat, 2013-01-12 at 15:35 +0000, Matt Wilson wrote:
> >> > 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.

[PATCH 4.1-testing] x86/mm: Fix loop increment in paging_log_dirty_range()

[Tim Deegan]

# HG changeset patch
# User Tim Deegan <tim@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 <snoonan@amazon.com>
Reported-by: Matt Wilson <msw@amazon.com>
Signed-off-by: Tim Deegan <tim@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) )

Re: [PATCH 4.1-testing] x86/mm: Fix loop increment in paging_log_dirty_range()

[Ian Campbell]

On Thu, 2013-01-17 at 11:30 +0000, Tim Deegan wrote:
> # HG changeset patch
> # User Tim Deegan <tim@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 <snoonan@amazon.com>
> Reported-by: Matt Wilson <msw@amazon.com>
> Signed-off-by: Tim Deegan <tim@xen.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> 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
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel