Fix tests for start+len address valid for guest

PATCH Applied Richard Henderson 3 participants 12 messages Apr 5, 2010 – May 6, 2010

Share & tools

Richard Henderson

author

2010-04-05 17:37 UTC #0 lore raw

cover Fix tests for start+len address valid for guest

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.

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(-)

Richard Henderson

author

2010-04-05 17:37 UTC #1 lore raw

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

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

Signed-off-by Richard Henderson <[email protected]>

---
 target-sparc/cpu.h |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

target-sparc/cpu.h +6 -6

index 580f4d4..0e7f390 100644
@@ -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

Signature · 1 line

1.6.6.1

Richard Henderson

author

2010-04-05 17:37 UTC #2 lore raw

PATCH 2/7 Add guest_start_len_valid function.

In reply to 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 <[email protected]>

---
 cpu-all.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

cpu-all.h +10 -0

index 927445c..7d31b16 100644
@@ -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) ({ \

Signature · 1 line

1.6.6.1

malc

2010-04-05 18:15 UTC #3 lore raw

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

Quoted 23 lines · 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 <[email protected]>
---
 cpu-all.h |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
cpu-all.h +5 -0
index 927445c..7d31b16 100644
@@ -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

cpu-all.h extern int have_guest_base; · line 655

Quoted 10 lines

cpu-all.h +5 -0

 #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.

Quoted 3 lines

cpu-all.h +0 -0

 #define h2g(x) ({ \

Signature · 1 line

mailto:[email protected]

Richard Henderson

author

2010-04-05 18:31 UTC #4 lore raw

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

Quoted 1 line · malc

Please do not use double leading underscore.

In contrast to the existing use in:

Quoted 2 lines

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

What do you suggest instead? Trailing underscores?

malc

2010-04-05 18:49 UTC #5 lore raw

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

Quoted 9 lines · Richard Henderson

Quoted 1 line · malc
Please do not use double leading underscore.
In contrast to the existing use in:
Quoted 2 lines
Quoted 2 lines
 #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.

Signature · 1 line

mailto:[email protected]

Richard Henderson

author

2010-04-05 17:37 UTC #6 lore raw

PATCH 3/7 linux-user: Use guest_start_len_valid in msync.

In reply to Richard Henderson

Make sure to properly handle len = 0 first.

Signed-off-by Richard Henderson <[email protected]>

---
 linux-user/mmap.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

linux-user/mmap.c +8 -5

index 46923c7..f4d44a8 100644
@@ -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);
 }

Signature · 1 line

1.6.6.1

Richard Henderson

author

2010-04-05 17:37 UTC #7 lore raw

PATCH 4/7 linux-user: Use guest_start_len_valid in mremap.

In reply to Richard Henderson

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

Signed-off-by Richard Henderson <[email protected]>

---
 linux-user/mmap.c |   44 ++++++++++++++++++++++++--------------------
 1 files changed, 24 insertions(+), 20 deletions(-)

linux-user/mmap.c +24 ~~-20~~

index f4d44a8..463679d 100644
@@ -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;
 }

Signature · 1 line

1.6.6.1

Richard Henderson

author

2010-04-05 17:37 UTC #8 lore raw

PATCH 5/7 linux-user: Use guest_start_len_valid in mmap.

In reply to Richard Henderson

Signed-off-by Richard Henderson <[email protected]>

---
 linux-user/mmap.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

linux-user/mmap.c +2 -6

index 463679d..085030b 100644
@@ -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;
         }

Signature · 1 line

1.6.6.1

Richard Henderson

author

2010-04-05 17:37 UTC #9 lore raw

PATCH 6/7 Fix zero-length write(2).

In reply to Richard Henderson

Signed-off-by Richard Henderson <[email protected]>

---
 exec.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

exec.c +3 -0

index 33854e1..d69194c 100644
@@ -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;

Signature · 1 line

1.6.6.1

Aurelien Jarno

Applied

2010-05-06 04:46 UTC #10 lore raw

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

Quoted 1 line · Richard Henderson

Signed-off-by Richard Henderson <[email protected]>

Thanks, applied.

Quoted 24 lines

---
 exec.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

exec.c +3 -0

index 33854e1..d69194c 100644
@@ -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;

Signature · 1 line

1.6.6.1

Signature · 2 lines

Aurelien Jarno                          GPG: 1024D/F1BCDB73
[email protected]                 http://www.aurel32.net

Richard Henderson

author

2010-04-05 17:37 UTC #11 lore raw

PATCH 7/7 Use guest_start_len_valid in page_check_range.

In reply to 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 <[email protected]>

---
 exec.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

exec.c +1 -8

index d69194c..ed5eacf 100644
@@ -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;
     }

Signature · 1 line

1.6.6.1