[Qemu-devel] [PATCH 0/7] Fix tests for start+len address valid for guest

[Qemu-devel] [PATCH 0/7] Fix tests for start+len address valid for guest

[Richard Henderson]

The first patch in the series fixes a major think-o in the sparc port.
The 64 and 32-bit constants were reversed.  Fixing these are required
to avoid a build error in later patches.

For the actual problem, introduce a guest_start_len_valid macro similar
to the existing h2g_valid macro, where we compare the address range
against TARGET_VIRT_ADDR_SPACE_BITS (or ULONG_MAX, depending on the host).

Use this in 3 places in the linux-user memory handling functions, and
also in the generic page_check_range.  In the later case, also remove
a mis-conception that I had that page_check_range was already bounds
checked -- these values come directly from a guest syscall and so can
contain any random errant values.


r~


Richard Henderson (7):
  target-sparc: Fix TARGET_{PHYS,VIRT}_ADDR_SPACE_BITS.
  Add guest_start_len_valid function.
  linux-user: Use guest_start_len_valid in msync.
  linux-user: Use guest_start_len_valid in mremap.
  linux-user: Use guest_start_len_valid in mmap.
  Fix zero-length write(2).
  Use guest_start_len_valid in page_check_range.

 cpu-all.h          |   10 ++++++++
 exec.c             |   12 +++------
 linux-user/mmap.c  |   65 +++++++++++++++++++++++++++------------------------
 target-sparc/cpu.h |   12 ++++----
 4 files changed, 54 insertions(+), 45 deletions(-)

[Qemu-devel] [PATCH 6/7] Fix zero-length write(2).

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 exec.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/exec.c b/exec.c
index 33854e1..d69194c 100644
--- a/exec.c
+++ b/exec.c
@@ -2461,6 +2461,9 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
     assert(start < ((abi_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
 #endif
 
+    if (len == 0) {
+        return 0;
+    }
     if (start + len - 1 < start) {
         /* We've wrapped around.  */
         return -1;
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH 6/7] Fix zero-length write(2).

[Aurelien Jarno]

On Mon, Mar 29, 2010 at 10:54:42AM -0700, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>

Thanks, applied.

> ---
>  exec.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 33854e1..d69194c 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2461,6 +2461,9 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
>      assert(start < ((abi_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
>  #endif
>  
> +    if (len == 0) {
> +        return 0;
> +    }
>      if (start + len - 1 < start) {
>          /* We've wrapped around.  */
>          return -1;
> -- 
> 1.6.6.1
> 
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH 1/7] target-sparc: Fix TARGET_{PHYS, VIRT}_ADDR_SPACE_BITS.

[Richard Henderson]

The 32 and 64-bit definitions were swapped in the ifdef.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target-sparc/cpu.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index 580f4d4..0e7f390 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -7,18 +7,18 @@
 #define TARGET_LONG_BITS 32
 #define TARGET_FPREGS 32
 #define TARGET_PAGE_BITS 12 /* 4k */
+#define TARGET_PHYS_ADDR_SPACE_BITS 36
+#define TARGET_VIRT_ADDR_SPACE_BITS 32
+#else
+#define TARGET_LONG_BITS 64
+#define TARGET_FPREGS 64
+#define TARGET_PAGE_BITS 13 /* 8k */
 #define TARGET_PHYS_ADDR_SPACE_BITS 41
 # ifdef TARGET_ABI32
 #  define TARGET_VIRT_ADDR_SPACE_BITS 32
 # else
 #  define TARGET_VIRT_ADDR_SPACE_BITS 44
 # endif
-#else
-#define TARGET_LONG_BITS 64
-#define TARGET_FPREGS 64
-#define TARGET_PAGE_BITS 13 /* 8k */
-#define TARGET_PHYS_ADDR_SPACE_BITS 36
-#define TARGET_VIRT_ADDR_SPACE_BITS 32
 #endif
 
 #define CPUState struct CPUSPARCState
-- 
1.6.6.1

[Qemu-devel] [PATCH 3/7] linux-user: Use guest_start_len_valid in msync.

[Richard Henderson]

Make sure to properly handle len = 0 first.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/mmap.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 46923c7..f4d44a8 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -706,15 +706,18 @@ int target_msync(abi_ulong start, abi_ulong len, int flags)
 {
     abi_ulong end;
 
-    if (start & ~TARGET_PAGE_MASK)
+    if (start & ~TARGET_PAGE_MASK) {
         return -EINVAL;
+    }
+    if (len == 0) {
+        return 0;
+    }
     len = TARGET_PAGE_ALIGN(len);
-    end = start + len;
-    if (end < start)
+    if (!guest_start_len_valid(start, len)) {
         return -EINVAL;
-    if (end == start)
-        return 0;
+    }
 
+    end = start + len;
     start &= qemu_host_page_mask;
     return msync(g2h(start), end - start, flags);
 }
-- 
1.6.6.1

[Qemu-devel] [PATCH 4/7] linux-user: Use guest_start_len_valid in mremap.

[Richard Henderson]

Also properly signal error for non-page aligned inputs
and zero sized outputs.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/mmap.c |   44 ++++++++++++++++++++++++--------------------
 1 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index f4d44a8..463679d 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -657,37 +657,40 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
                        abi_ulong new_addr)
 {
     int prot;
-    void *host_addr;
+    void *host_addr = MAP_FAILED;
 
-    mmap_lock();
+    if (new_size == 0 || (old_addr & ~TARGET_PAGE_MASK)) {
+        errno = EINVAL;
+        return -1;
+    }
 
-    if (flags & MREMAP_FIXED)
-        host_addr = (void *) syscall(__NR_mremap, g2h(old_addr),
-                                     old_size, new_size,
-                                     flags,
-                                     new_addr);
-    else if (flags & MREMAP_MAYMOVE) {
-        abi_ulong mmap_start;
+    mmap_lock();
 
-        mmap_start = mmap_find_vma(0, new_size);
+    /* ??? If host page size > target page size, we can fail here
+       when we shouldn't.  */
 
+    if (flags & MREMAP_FIXED) {
+        if (new_addr & ~TARGET_PAGE_MASK) {
+            errno = EINVAL;
+        } else {
+            host_addr = (void *) syscall(__NR_mremap, g2h(old_addr),
+                                         old_size, new_size, flags, new_addr);
+        }
+    } else if (flags & MREMAP_MAYMOVE) {
+        abi_ulong mmap_start = mmap_find_vma(0, new_size);
         if (mmap_start == -1) {
             errno = ENOMEM;
-            host_addr = MAP_FAILED;
-        } else
+        } else {
             host_addr = (void *) syscall(__NR_mremap, g2h(old_addr),
                                          old_size, new_size,
                                          flags | MREMAP_FIXED,
                                          g2h(mmap_start));
-    } else {
-        host_addr = mremap(g2h(old_addr), old_size, new_size, flags);
-        /* Check if address fits target address space */
-        if ((unsigned long)host_addr + new_size > (abi_ulong)-1) {
-            /* Revert mremap() changes */
-            host_addr = mremap(g2h(old_addr), new_size, old_size, flags);
-            errno = ENOMEM;
-            host_addr = MAP_FAILED;
         }
+    } else if (guest_start_len_valid(old_addr, new_size)) {
+        host_addr = mremap(g2h(old_addr), old_size, new_size, flags);
+        assert(host_addr == g2h(old_addr) || host_addr == MAP_FAILED);
+    } else {
+        errno = ENOMEM;
     }
 
     if (host_addr == MAP_FAILED) {
@@ -698,6 +701,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
         page_set_flags(old_addr, old_addr + old_size, 0);
         page_set_flags(new_addr, new_addr + new_size, prot | PAGE_VALID);
     }
+
     mmap_unlock();
     return new_addr;
 }
-- 
1.6.6.1

[Qemu-devel] [PATCH 5/7] linux-user: Use guest_start_len_valid in mmap.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/mmap.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index 463679d..085030b 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -494,12 +494,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
         end = start + len;
         real_end = HOST_PAGE_ALIGN(end);
 
-	/*
-	 * Test if requested memory area fits target address space
-	 * It can fail only on 64-bit host with 32-bit target.
-	 * On any other target/host host mmap() handles this error correctly.
-	 */
-        if ((unsigned long)start + len - 1 > (abi_ulong) -1) {
+	/* Test if requested memory area fits target address space.  */
+        if (!guest_start_len_valid(start, len)) {
             errno = EINVAL;
             goto fail;
         }
-- 
1.6.6.1

[Qemu-devel] [PATCH 7/7] Use guest_start_len_valid in page_check_range.

[Richard Henderson]

Also remove an assertion on start being in range.  The values
here can come directly from the guest via a syscall, and so
very well may be out of range via plain bug or DoS attack.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 exec.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/exec.c b/exec.c
index d69194c..ed5eacf 100644
--- a/exec.c
+++ b/exec.c
@@ -2454,17 +2454,10 @@ int page_check_range(target_ulong start, target_ulong len, int flags)
     target_ulong end;
     target_ulong addr;
 
-    /* This function should never be called with addresses outside the
-       guest address space.  If this assert fires, it probably indicates
-       a missing call to h2g_valid.  */
-#if TARGET_ABI_BITS > L1_MAP_ADDR_SPACE_BITS
-    assert(start < ((abi_ulong)1 << L1_MAP_ADDR_SPACE_BITS));
-#endif
-
     if (len == 0) {
         return 0;
     }
-    if (start + len - 1 < start) {
+    if (!guest_start_len_valid(start, len)) {
         /* We've wrapped around.  */
         return -1;
     }
-- 
1.6.6.1

[Qemu-devel] [PATCH 2/7] Add guest_start_len_valid function.

[Richard Henderson]

To be used by userspace emulation to verify that the memory
range defined by [start, start+len) is valid for the guest,
taking into account TARGET_VIRT_ADDR_SPACE_BITS.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 cpu-all.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index 927445c..7d31b16 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -637,11 +637,21 @@ extern int have_guest_base;
 
 #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
 #define h2g_valid(x) 1
+#define guest_start_len_valid(s,l)  ({          \
+    unsigned long __s = (unsigned long)(s);     \
+    unsigned long __e = __s + (l) - 1;          \
+    __e >= __s;                                 \
+})
 #else
 #define h2g_valid(x) ({ \
     unsigned long __guest = (unsigned long)(x) - GUEST_BASE; \
     __guest < (1ul << TARGET_VIRT_ADDR_SPACE_BITS); \
 })
+#define guest_start_len_valid(s,l) ({                           \
+    unsigned long __s = (unsigned long)(s);                     \
+    unsigned long __e = __s + (l) - 1;                          \
+    __e >= __s && __e < (1ul << TARGET_VIRT_ADDR_SPACE_BITS);   \
+})
 #endif
 
 #define h2g(x) ({ \
-- 
1.6.6.1

Re: [Qemu-devel] [PATCH 2/7] Add guest_start_len_valid function.

[malc]

On Mon, 5 Apr 2010, Richard Henderson wrote:

> To be used by userspace emulation to verify that the memory
> range defined by [start, start+len) is valid for the guest,
> taking into account TARGET_VIRT_ADDR_SPACE_BITS.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  cpu-all.h |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index 927445c..7d31b16 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -637,11 +637,21 @@ extern int have_guest_base;
>  
>  #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
>  #define h2g_valid(x) 1
> +#define guest_start_len_valid(s,l)  ({          \
> +    unsigned long __s = (unsigned long)(s);     \
> +    unsigned long __e = __s + (l) - 1;          \
> +    __e >= __s;                                 \
> +})
>  #else
>  #define h2g_valid(x) ({ \
>      unsigned long __guest = (unsigned long)(x) - GUEST_BASE; \
>      __guest < (1ul << TARGET_VIRT_ADDR_SPACE_BITS); \
>  })
> +#define guest_start_len_valid(s,l) ({                           \
> +    unsigned long __s = (unsigned long)(s);                     \
> +    unsigned long __e = __s + (l) - 1;                          \
> +    __e >= __s && __e < (1ul << TARGET_VIRT_ADDR_SPACE_BITS);   \
> +})
>  #endif

Please do not use double leading underscore.

>  
>  #define h2g(x) ({ \
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/7] Add guest_start_len_valid function.

[Richard Henderson]

On 04/05/2010 11:15 AM, malc wrote:
> Please do not use double leading underscore.

In contrast to the existing use in:

>>  #define h2g_valid(x) ({ \
>>      unsigned long __guest = (unsigned long)(x) - GUEST_BASE; \

What do you suggest instead?  Trailing underscores?


r~

Re: [Qemu-devel] [PATCH 2/7] Add guest_start_len_valid function.

[malc]

On Mon, 5 Apr 2010, Richard Henderson wrote:

> On 04/05/2010 11:15 AM, malc wrote:
> > Please do not use double leading underscore.
> 
> In contrast to the existing use in:
> 
> >>  #define h2g_valid(x) ({ \
> >>      unsigned long __guest = (unsigned long)(x) - GUEST_BASE; \
> 
> What do you suggest instead?  Trailing underscores?

Yes, the code is not C++ so trailing underscores will do.

-- 
mailto:av1474@comtv.ru