[PATCH] Xen: fix various checks of unsigned integers < 0

[PATCH] Xen: fix various checks of unsigned integers < 0

[Tim Deegan]

# HG changeset patch
# User Tim Deegan <Tim.Deegan@citrix.com>
# Date 1288360674 -3600
# Node ID 52ce5ef855cf2582749d2d0812754ac074cc14e9
# Parent  3cc0fac4a49e29ca0b841c8354a0a9c0686e3d58
Xen: fix various checks of unsigned integers < 0

Some of these could be benignly discarded by the compiler but some are
actual bugs.

Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>

diff -r 3cc0fac4a49e -r 52ce5ef855cf xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c	Fri Oct 29 14:57:50 2010 +0100
+++ b/xen/arch/x86/mm.c	Fri Oct 29 14:57:54 2010 +0100
@@ -4533,7 +4533,7 @@ static int handle_iomem_range(unsigned l
         ent.size = (uint64_t)(s - ctxt->s) << PAGE_SHIFT;
         ent.type = E820_RESERVED;
         buffer = guest_handle_cast(ctxt->map.buffer, e820entry_t);
-        if ( __copy_to_guest_offset(buffer, ctxt->n, &ent, 1) < 0 )
+        if ( __copy_to_guest_offset(buffer, ctxt->n, &ent, 1) )
             return -EFAULT;
         ctxt->n++;
     }
@@ -4750,7 +4750,7 @@ long arch_memory_op(int op, XEN_GUEST_HA
             }
             if ( ctxt.map.nr_entries <= ctxt.n + (e820.nr_map - i) )
                 return -EINVAL;
-            if ( __copy_to_guest_offset(buffer, ctxt.n, e820.map + i, 1) < 0 )
+            if ( __copy_to_guest_offset(buffer, ctxt.n, e820.map + i, 1) )
                 return -EFAULT;
             ctxt.s = PFN_UP(e820.map[i].addr + e820.map[i].size);
         }
diff -r 3cc0fac4a49e -r 52ce5ef855cf xen/arch/x86/physdev.c
--- a/xen/arch/x86/physdev.c	Fri Oct 29 14:57:50 2010 +0100
+++ b/xen/arch/x86/physdev.c	Fri Oct 29 14:57:54 2010 +0100
@@ -202,7 +202,7 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
         if ( copy_from_guest(&eoi, arg, 1) != 0 )
             break;
         ret = -EINVAL;
-        if ( eoi.irq < 0 || eoi.irq >= v->domain->nr_pirqs )
+        if ( eoi.irq >= v->domain->nr_pirqs )
             break;
         if ( v->domain->arch.pirq_eoi_map )
             evtchn_unmask(v->domain->pirq_to_evtchn[eoi.irq]);
diff -r 3cc0fac4a49e -r 52ce5ef855cf xen/arch/x86/platform_hypercall.c
--- a/xen/arch/x86/platform_hypercall.c	Fri Oct 29 14:57:50 2010 +0100
+++ b/xen/arch/x86/platform_hypercall.c	Fri Oct 29 14:57:54 2010 +0100
@@ -418,7 +418,6 @@ ret_t do_platform_op(XEN_GUEST_HANDLE(xe
         }
 
         if ( (g_info->xen_cpuid >= NR_CPUS) ||
-             (g_info->xen_cpuid < 0) ||
              !cpu_present(g_info->xen_cpuid) )
         {
             g_info->flags |= XEN_PCPU_FLAGS_INVALID;
diff -r 3cc0fac4a49e -r 52ce5ef855cf xen/arch/x86/x86_emulate/x86_emulate.c
--- a/xen/arch/x86/x86_emulate/x86_emulate.c	Fri Oct 29 14:57:50 2010 +0100
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c	Fri Oct 29 14:57:54 2010 +0100
@@ -2102,7 +2102,7 @@ x86_emulate(
             _regs.edx = (uint32_t)(((int32_t)_regs.eax < 0) ? -1 : 0);
             break;
         case 8:
-            _regs.edx = (_regs.eax < 0) ? -1 : 0;
+            _regs.edx = ((int64_t)_regs.eax < 0) ? -1 : 0;
             break;
         }
         break;
diff -r 3cc0fac4a49e -r 52ce5ef855cf xen/drivers/cpufreq/cpufreq.c
--- a/xen/drivers/cpufreq/cpufreq.c	Fri Oct 29 14:57:50 2010 +0100
+++ b/xen/drivers/cpufreq/cpufreq.c	Fri Oct 29 14:57:54 2010 +0100
@@ -116,8 +116,7 @@ int cpufreq_limit_change(unsigned int cp
         !processor_pminfo[cpu])
         return -ENODEV;
 
-    if ((perf->platform_limit < 0) || 
-        (perf->platform_limit >= perf->state_count))
+    if (perf->platform_limit >= perf->state_count)
         return -EINVAL;
 
     memcpy(&policy, data, sizeof(struct cpufreq_policy)); 

RE: [PATCH] Xen: fix various checks of unsigned integers < 0

[Dan Magenheimer]

> diff -r 3cc0fac4a49e -r 52ce5ef855cf
> xen/arch/x86/x86_emulate/x86_emulate.c
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c	Fri Oct 29 14:57:50
> 2010 +0100
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c	Fri Oct 29 14:57:54
> 2010 +0100
> @@ -2102,7 +2102,7 @@ x86_emulate(
>              _regs.edx = (uint32_t)(((int32_t)_regs.eax < 0) ? -1 : 0);
>              break;
>          case 8:
> -            _regs.edx = (_regs.eax < 0) ? -1 : 0;
> +            _regs.edx = ((int64_t)_regs.eax < 0) ? -1 : 0;
>              break;
>          }
>          break;

(/me goes and looks up the cwd instruction...)

Wow, I wonder how many times this code has executed
and returned the wrong (incorrectly sign-extended) value?
Talk about a possible silent-but-deadly bug that would
be impossible to track down!

Nice catch!  Future Xen support people thank you!

Re: [PATCH] Xen: fix various checks of unsigned integers < 0

[Paolo Bonzini]

On 10/29/2010 05:38 PM, Dan Magenheimer wrote:
> Wow, I wonder how many times this code has executed
> and returned the wrong (incorrectly sign-extended) value?

Probably never---which doesn't make the fix worthless, but is still 
never. :)  The emulator is mostly used for real mode and MMIO, but this 
is long-mode code (which rules out real mode) and the CQO instruction 
doesn't access memory (which rules out MMIO).

To trigger the bug you probably have to cause a race between a thread 
doing MMIO and a thread replacing the MMIO instruction with a CQO.  It 
can be done fairly reliably on KVM; until they were patched, this trick 
allowed to exploit emulator bugs and go from guest-ring3 to guest-ring0.

Paolo