Skip to contentSkip to conversation

linux-user: fix mmap/munmap/mprotect/mremap/shmat

PATCH Superseded Max Filippov 2 participants 3 messages Mar 6, 2018

v4 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Max Filippov

author
#0 lore raw

PATCH v4 5/11 linux-user: fix mmap/munmap/mprotect/mremap/shmat

In linux-user QEMU that runs for a target with TARGET_ABI_BITS bigger than L1_MAP_ADDR_SPACE_BITS an assertion in page_set_flags fires when mmap, munmap, mprotect, mremap or shmat is called for an address outside the guest address space. mmap and mprotect should return ENOMEM in such case.

Introduce macro guest_range_valid that verifies if address range is within guest address space and does not wrap around. Use that macro in mmap/munmap/mprotect/mremap/shmat for error checking.

Cc [email protected]
Cc Riku Voipio <[email protected]>
Cc Laurent Vivier <[email protected]>
Signed-off-by Max Filippov <[email protected]>

---
Changes v3->v4:
- change GUEST_ADDR_MAX and h2g_valid definitions as suggested by Laurent Vivier.

Changes v2->v3:
- fix comparison in guest_valid: it must be 'less' to preserve the existing functionality, not 'less or equal'.
- fix guest_range_valid: it may not use guest_valid, because single range that occupies all of the guest address space is valid.

 include/exec/cpu-all.h  |  4 ++++
 include/exec/cpu_ldst.h | 14 ++++++++------
 linux-user/mmap.c       | 20 +++++++++++++++-----
 linux-user/syscall.c    |  3 +++
 4 files changed, 30 insertions(+), 11 deletions(-)
include/exec/cpu-all.h +4 -0
index 0b141683f095..6304cfa7e171 100644
@@ -159,8 +159,12 @@ extern unsigned long guest_base;
 extern int have_guest_base;
 extern unsigned long reserved_va;
 
+#if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
+#define GUEST_ADDR_MAX (reserved_va ? reserved_va : ~0ul)
+#else
 #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
                                     (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
+#endif
 #else
 
 #include "exec/hwaddr.h"
include/exec/cpu_ldst.h +8 -6
index 191f2e962a3c..22f5df9c8a92 100644
@@ -52,15 +52,17 @@
 #define g2h(x) ((void *)((unsigned long)(target_ulong)(x) + guest_base))
 
 #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
-#define h2g_valid(x) 1
+#define guest_valid(x) 1
 #else
-#define h2g_valid(x) ({ \
-    unsigned long __guest = (unsigned long)(x) - guest_base; \
-    (__guest < (1ul << TARGET_VIRT_ADDR_SPACE_BITS)) && \
-    (!reserved_va || (__guest < reserved_va)); \
-})
+#define guest_valid(x) ((x) < GUEST_ADDR_MAX)
 #endif
 
+#define h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)
+
+#define guest_range_valid(start, len) \
+    ({unsigned long l = (len); \
+     l <= GUEST_ADDR_MAX && (start) <= GUEST_ADDR_MAX - l; })
+
 #define h2g_nocheck(x) ({ \
     unsigned long __ret = (unsigned long)(x) - guest_base; \
     (abi_ulong)__ret; \
linux-user/mmap.c +15 -5
index 0fbfd6dff20d..df81f9b803b6 100644
@@ -80,8 +80,9 @@ int target_mprotect(abi_ulong start, abi_ulong len, int prot)
         return -EINVAL;
     len = TARGET_PAGE_ALIGN(len);
     end = start + len;
-    if (end < start)
-        return -EINVAL;
+    if (!guest_range_valid(start, len)) {
+        return -ENOMEM;
+    }
     prot &= PROT_READ | PROT_WRITE | PROT_EXEC;
     if (len == 0)
         return 0;

@@ -481,8 +482,8 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot,
 	 * 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) {
-            errno = EINVAL;
+        if (!guest_range_valid(start, len)) {
+            errno = ENOMEM;
             goto fail;
         }
 

@@ -622,8 +623,10 @@ int target_munmap(abi_ulong start, abi_ulong len)
     if (start & ~TARGET_PAGE_MASK)
         return -EINVAL;
     len = TARGET_PAGE_ALIGN(len);
-    if (len == 0)
+    if (len == 0 || !guest_range_valid(start, len)) {
         return -EINVAL;
+    }
+
     mmap_lock();
     end = start + len;
     real_start = start & qemu_host_page_mask;

@@ -678,6 +681,13 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong old_size,
     int prot;
     void *host_addr;
 
+    if (!guest_range_valid(old_addr, old_size) ||
+        ((flags & MREMAP_FIXED) &&
+         !guest_range_valid(new_addr, new_size))) {
+        errno = ENOMEM;
+        return -1;
+    }
+
     mmap_lock();
 
     if (flags & MREMAP_FIXED) {
linux-user/syscall.c +3 -0
index e24f43c4a259..79245e73784f 100644
@@ -4900,6 +4900,9 @@ static inline abi_ulong do_shmat(CPUArchState *cpu_env,
             return -TARGET_EINVAL;
         }
     }
+    if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {
+        return -TARGET_EINVAL;
+    }
 
     mmap_lock();
Signature · 1 line
2.11.0
#1 lore raw

Le 06/03/2018 à 20:34, Max Filippov a écrit :

Quoted 36 lines

In linux-user QEMU that runs for a target with TARGET_ABI_BITS bigger than L1_MAP_ADDR_SPACE_BITS an assertion in page_set_flags fires when mmap, munmap, mprotect, mremap or shmat is called for an address outside the guest address space. mmap and mprotect should return ENOMEM in such case.

Introduce macro guest_range_valid that verifies if address range is within guest address space and does not wrap around. Use that macro in mmap/munmap/mprotect/mremap/shmat for error checking.

Cc [email protected]
Cc Riku Voipio <[email protected]>
Cc Laurent Vivier <[email protected]>
Signed-off-by Max Filippov <[email protected]>

---
Changes v3->v4:
- change GUEST_ADDR_MAX and h2g_valid definitions as suggested by Laurent Vivier.

Changes v2->v3:
- fix comparison in guest_valid: it must be 'less' to preserve the existing functionality, not 'less or equal'.
- fix guest_range_valid: it may not use guest_valid, because single range that occupies all of the guest address space is valid.

 include/exec/cpu-all.h  |  4 ++++
 include/exec/cpu_ldst.h | 14 ++++++++------
 linux-user/mmap.c       | 20 +++++++++++++++-----
 linux-user/syscall.c    |  3 +++
 4 files changed, 30 insertions(+), 11 deletions(-)
include/exec/cpu-all.h +0 -0
index 0b141683f095..6304cfa7e171 100644

include/exec/cpu-all.h extern unsigned long guest_base; · line 163

Quoted 5 lines
include/exec/cpu-all.h +2 -0
@@ -159,8 +159,12 @@ extern unsigned long guest_base;
 extern int have_guest_base;
 extern unsigned long reserved_va;
 
+#if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
+#define GUEST_ADDR_MAX (reserved_va ? reserved_va : ~0ul)

In fact, below, for h2g_valid(), reserved_va is ignored in this case, so it should be:

#define GUEST_ADDR_MAX (~0ul)

[I know, my bad]

include/exec/cpu-all.h extern unsigned long guest_base; · line 165

Quoted 2 lines
include/exec/cpu-all.h +1 -0
+#else
 #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \

I think it should become "reserved_va ? reserved_va - 1 : \" as "reserved_va" is a size but GUEST_ADDR_MAX is the maximum value available. See below.

Quoted 12 lines
include/exec/cpu-all.h +1 -0
                                     (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
+#endif
 #else
 
 #include "exec/hwaddr.h"
include/exec/cpu_ldst.h +0 -0
index 191f2e962a3c..22f5df9c8a92 100644
@@ -52,15 +52,17 @@
 #define g2h(x) ((void *)((unsigned long)(target_ulong)(x) + guest_base))

include/exec/cpu_ldst.h line 57

Quoted 10 lines
include/exec/cpu_ldst.h +2 -6
 #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
-#define h2g_valid(x) 1
+#define guest_valid(x) 1
 #else
-#define h2g_valid(x) ({ \
-    unsigned long __guest = (unsigned long)(x) - guest_base; \
-    (__guest < (1ul << TARGET_VIRT_ADDR_SPACE_BITS)) && \
-    (!reserved_va || (__guest < reserved_va)); \
-})
+#define guest_valid(x) ((x) < GUEST_ADDR_MAX)

I think it should be ((x) <= GUEST_ADDR_MAX), because

    (__guest < (1ul << TARGET_VIRT_ADDR_SPACE_BITS))

-> (__guest <= ((1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1) -> (__guest <= GUEST_ADDR_MAX)

To work with reserved_va, it has also to be defined as "reserved_va - 1". And in open_self_maps() we should have "max = ... :
(uintptr_t)g2h(GUEST_ADDR_MAX) + 1;" and then we have a "h2g(max - 1)" that will correctly return GUEST_ADDR_MAX.

Then you don't need the "#if" because if "HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS", x is 32bit and GUEST_ADDR_MAX) is ~0uk, and then ((x) <= GUEST_ADDR_MAX) is always true.

Quoted 11 lines
include/exec/cpu_ldst.h +6 -0
 #endif
 
+#define h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)
+
+#define guest_range_valid(start, len) \
+    ({unsigned long l = (len); \
+     l <= GUEST_ADDR_MAX && (start) <= GUEST_ADDR_MAX - l; })
+
 #define h2g_nocheck(x) ({ \
     unsigned long __ret = (unsigned long)(x) - guest_base; \
     (abi_ulong)__ret; \

...

linux-user/syscall.c static inline abi_ulong do_shmat(CPUArchState *cpu_env, · line 4903

Quoted 9 lines
linux-user/syscall.c +1 -0
index e24f43c4a259..79245e73784f 100644
@@ -4900,6 +4900,9 @@ static inline abi_ulong do_shmat(CPUArchState *cpu_env,
             return -TARGET_EINVAL;
         }
     }
+    if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {

if shmaddr is NULL, "the system chooses a suitable (unused) address" so you can't check this as is.

Thanks,
Laurent

author
#2 lore raw
Quoted 48 lines · Laurent Vivier
Quoted 36 lines · Max Filippov

In linux-user QEMU that runs for a target with TARGET_ABI_BITS bigger than L1_MAP_ADDR_SPACE_BITS an assertion in page_set_flags fires when mmap, munmap, mprotect, mremap or shmat is called for an address outside the guest address space. mmap and mprotect should return ENOMEM in such case.

Introduce macro guest_range_valid that verifies if address range is within guest address space and does not wrap around. Use that macro in mmap/munmap/mprotect/mremap/shmat for error checking.

Cc [email protected]
Cc Riku Voipio <[email protected]>
Cc Laurent Vivier <[email protected]>
Signed-off-by Max Filippov <[email protected]>

---
Changes v3->v4:
- change GUEST_ADDR_MAX and h2g_valid definitions as suggested by Laurent Vivier.

Changes v2->v3:
- fix comparison in guest_valid: it must be 'less' to preserve the existing functionality, not 'less or equal'.
- fix guest_range_valid: it may not use guest_valid, because single range that occupies all of the guest address space is valid.

 include/exec/cpu-all.h  |  4 ++++
 include/exec/cpu_ldst.h | 14 ++++++++------
 linux-user/mmap.c       | 20 +++++++++++++++-----
 linux-user/syscall.c    |  3 +++
 4 files changed, 30 insertions(+), 11 deletions(-)
include/exec/cpu-all.h +0 -0
index 0b141683f095..6304cfa7e171 100644

include/exec/cpu-all.h extern unsigned long guest_base; · line 163

Quoted 5 lines
include/exec/cpu-all.h +2 -0
@@ -159,8 +159,12 @@ extern unsigned long guest_base;
 extern int have_guest_base;
 extern unsigned long reserved_va;
 
+#if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
+#define GUEST_ADDR_MAX (reserved_va ? reserved_va : ~0ul)

In fact, below, for h2g_valid(), reserved_va is ignored in this case, so it should be:

#define GUEST_ADDR_MAX (~0ul)

Quoted 8 lines

[I know, my bad]

include/exec/cpu-all.h extern unsigned long guest_base; · line 165

Quoted 2 lines
include/exec/cpu-all.h +1 -0
+#else
 #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \

I think it should become "reserved_va ? reserved_va - 1 : \" as "reserved_va" is a size but GUEST_ADDR_MAX is the maximum value available. See below.

Agree.

Quoted 25 lines
Quoted 12 lines
include/exec/cpu-all.h +1 -0
                                     (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
+#endif
 #else
 
 #include "exec/hwaddr.h"
include/exec/cpu_ldst.h +0 -0
index 191f2e962a3c..22f5df9c8a92 100644
@@ -52,15 +52,17 @@
 #define g2h(x) ((void *)((unsigned long)(target_ulong)(x) + guest_base))

include/exec/cpu_ldst.h line 57

Quoted 10 lines
include/exec/cpu_ldst.h +2 -6
 #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
-#define h2g_valid(x) 1
+#define guest_valid(x) 1
 #else
-#define h2g_valid(x) ({ \
-    unsigned long __guest = (unsigned long)(x) - guest_base; \
-    (__guest < (1ul << TARGET_VIRT_ADDR_SPACE_BITS)) && \
-    (!reserved_va || (__guest < reserved_va)); \
-})
+#define guest_valid(x) ((x) < GUEST_ADDR_MAX)

I think it should be ((x) <= GUEST_ADDR_MAX), because

Quoted 3 lines
    (__guest < (1ul << TARGET_VIRT_ADDR_SPACE_BITS))

-> (__guest <= ((1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1) -> (__guest <= GUEST_ADDR_MAX)

Ok

Quoted 4 lines

To work with reserved_va, it has also to be defined as "reserved_va - 1". And in open_self_maps() we should have "max = ... :
(uintptr_t)g2h(GUEST_ADDR_MAX) + 1;" and then we have a "h2g(max - 1)" that will correctly return GUEST_ADDR_MAX.

Ok

Quoted 3 lines

Then you don't need the "#if" because if "HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS", x is 32bit and GUEST_ADDR_MAX) is ~0uk, and then ((x) <= GUEST_ADDR_MAX) is always true.

Ok.

Quoted 12 lines
Quoted 11 lines
include/exec/cpu_ldst.h +6 -0
 #endif
 
+#define h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)
+
+#define guest_range_valid(start, len) \
+    ({unsigned long l = (len); \
+     l <= GUEST_ADDR_MAX && (start) <= GUEST_ADDR_MAX - l; })
+
 #define h2g_nocheck(x) ({ \
     unsigned long __ret = (unsigned long)(x) - guest_base; \
     (abi_ulong)__ret; \

...

Quoted 12 lines

linux-user/syscall.c static inline abi_ulong do_shmat(CPUArchState *cpu_env, · line 4903

Quoted 9 lines
linux-user/syscall.c +1 -0
index e24f43c4a259..79245e73784f 100644
@@ -4900,6 +4900,9 @@ static inline abi_ulong do_shmat(CPUArchState *cpu_env,
             return -TARGET_EINVAL;
         }
     }
+    if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {

if shmaddr is NULL, "the system chooses a suitable (unused) address" so you can't check this as is.

Why not? guest_range_valid will be true for shmaddr == NULL and all valid sizes.

Signature · 2 lines
Thanks.
-- Max