Skip to contentSkip to conversation

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

PATCH Superseded Max Filippov 2 participants 5 messages Mar 1, 2018 – Mar 6, 2018

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Max Filippov

author
#0 lore raw

PATCH v3 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 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.

These changes fix assertion in page_check_range called from open_self_maps.

 include/exec/cpu-all.h  |  2 +-
 include/exec/cpu_ldst.h | 12 +++++++-----
 linux-user/mmap.c       | 20 +++++++++++++++-----
 linux-user/syscall.c    |  3 +++
 4 files changed, 26 insertions(+), 11 deletions(-)
include/exec/cpu-all.h +1 -1
index 0b141683f095..12bd049997ac 100644
@@ -160,7 +160,7 @@ extern int have_guest_base;
 extern unsigned long reserved_va;
 
 #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
-                                    (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
+                        (2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)) - 1)
 #else
 
 #include "exec/hwaddr.h"
include/exec/cpu_ldst.h +7 -5
index 191f2e962a3c..6528fd829ffe 100644
@@ -53,14 +53,16 @@
 
 #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 h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)
+#define guest_valid(x) ((x) < GUEST_ADDR_MAX)
 #endif
 
+#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 01/03/2018 à 18:36, Max Filippov a écrit :

Quoted 34 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 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.

These changes fix assertion in page_check_range called from open_self_maps.

 include/exec/cpu-all.h  |  2 +-
 include/exec/cpu_ldst.h | 12 +++++++-----
 linux-user/mmap.c       | 20 +++++++++++++++-----
 linux-user/syscall.c    |  3 +++
 4 files changed, 26 insertions(+), 11 deletions(-)
include/exec/cpu-all.h +0 -0
index 0b141683f095..12bd049997ac 100644

include/exec/cpu-all.h extern int have_guest_base; · line 163

Quoted 5 lines
include/exec/cpu-all.h +1 -1
@@ -160,7 +160,7 @@ extern int have_guest_base;
 extern unsigned long reserved_va;
 
 #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
-                                    (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
+                        (2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)) - 1)

I don't understand why you do this change. Could you explain?

Quoted 11 lines
include/exec/cpu-all.h +0 -0
 #else
 
 #include "exec/hwaddr.h"
include/exec/cpu_ldst.h +0 -0
index 191f2e962a3c..6528fd829ffe 100644
@@ -53,14 +53,16 @@
 
 #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
 #define h2g_valid(x) 1

include/exec/cpu_ldst.h line 60

Quoted 10 lines
include/exec/cpu_ldst.h +3 -5
+#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 h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)
+#define guest_valid(x) ((x) < GUEST_ADDR_MAX)
 #endif

I think you can just define h2g_valid(x) after the "endif":

  #define h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)

it will be set according to the guest_valid() definition ("1" or the result of the evaluation).

Thanks,
Laurent

author
#2 lore raw
Quoted 37 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 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.

These changes fix assertion in page_check_range called from open_self_maps.

 include/exec/cpu-all.h  |  2 +-
 include/exec/cpu_ldst.h | 12 +++++++-----
 linux-user/mmap.c       | 20 +++++++++++++++-----
 linux-user/syscall.c    |  3 +++
 4 files changed, 26 insertions(+), 11 deletions(-)
include/exec/cpu-all.h +0 -0
index 0b141683f095..12bd049997ac 100644
@@ -160,7 +160,7 @@ extern int have_guest_base;
 extern unsigned long reserved_va;
Quoted 5 lines

include/exec/cpu-all.h extern int have_guest_base; · line 163

Quoted 3 lines
include/exec/cpu-all.h +1 -1
 #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
-                                    (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
+                        (2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)) - 1)

I don't understand why you do this change. Could you explain?

For a lot of targets TARGET_VIRT_ADDR_SPACE_BITS is the same as HOST_LONG_BITS, so the shift results in undefined behavior. E.g. without this change I see the following warnings when building such targets for x86_64 host:

linux-user/syscall.c:4905:10: note: in expansion of macro ‘guest_range_valid’

     if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {
          ^~~~~~~~~~~~~~~~~
include/exec/cpu-all.h:163:30: warning: left shift count >= width of
type [-Wshift-count-overflow]
                         (1ul << (TARGET_VIRT_ADDR_SPACE_BITS)) - 1)

Changing it to 2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1) fixes undefined behavior. Probably that deserves a comment in the changelog.

Quoted 24 lines
Quoted 11 lines
include/exec/cpu-all.h +0 -0
 #else
 
 #include "exec/hwaddr.h"
include/exec/cpu_ldst.h +0 -0
index 191f2e962a3c..6528fd829ffe 100644
@@ -53,14 +53,16 @@
 
 #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
 #define h2g_valid(x) 1

include/exec/cpu_ldst.h line 60

Quoted 10 lines
include/exec/cpu_ldst.h +3 -5
+#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 h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)
+#define guest_valid(x) ((x) < GUEST_ADDR_MAX)
 #endif

I think you can just define h2g_valid(x) after the "endif":

Quoted 4 lines
  #define h2g_valid(x) guest_valid((unsigned long)(x) - guest_base)

it will be set according to the guest_valid() definition ("1" or the result of the evaluation).

Ok, will do.

Signature · 2 lines
Thanks.
-- Max
#3 lore raw

Le 06/03/2018 à 18:28, Max Filippov a écrit :

Quoted 56 lines · Laurent Vivier
Quoted 37 lines · Max Filippov
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 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.

These changes fix assertion in page_check_range called from open_self_maps.

 include/exec/cpu-all.h  |  2 +-
 include/exec/cpu_ldst.h | 12 +++++++-----
 linux-user/mmap.c       | 20 +++++++++++++++-----
 linux-user/syscall.c    |  3 +++
 4 files changed, 26 insertions(+), 11 deletions(-)
include/exec/cpu-all.h +0 -0
index 0b141683f095..12bd049997ac 100644
@@ -160,7 +160,7 @@ extern int have_guest_base;
 extern unsigned long reserved_va;
Quoted 5 lines

include/exec/cpu-all.h extern int have_guest_base; · line 163

Quoted 3 lines
include/exec/cpu-all.h +1 -1
 #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
-                                    (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
+                        (2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)) - 1)

I don't understand why you do this change. Could you explain?

For a lot of targets TARGET_VIRT_ADDR_SPACE_BITS is the same as HOST_LONG_BITS, so the shift results in undefined behavior. E.g. without this change I see the following warnings when building such targets for x86_64 host:

linux-user/syscall.c:4905:10: note: in expansion of macro ‘guest_range_valid’

     if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {
          ^~~~~~~~~~~~~~~~~
include/exec/cpu-all.h:163:30: warning: left shift count >= width of
type [-Wshift-count-overflow]
                         (1ul << (TARGET_VIRT_ADDR_SPACE_BITS)) - 1)
Quoted 4 lines

Changing it to 2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1) fixes undefined behavior. Probably that deserves a comment in the changelog.

Perhaps you can keep the scheme used with Le 06/03/2018 à 18:28, Max

Quoted 12 lines · Filippov
Quoted 11 lines · Laurent Vivier
Quoted 10 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.

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

---
Changes v2->v3:
- fix comparison in guest_valid: it must be 'less' to preserve the

existing

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines

functionality, not 'less or equal'.
- fix guest_range_valid: it may not use guest_valid, because single

range

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

that occupies all of the guest address space is valid.

These changes fix assertion in page_check_range called from

open_self_maps.

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
 include/exec/cpu-all.h  |  2 +-
 include/exec/cpu_ldst.h | 12 +++++++-----
 linux-user/mmap.c       | 20 +++++++++++++++-----
 linux-user/syscall.c    |  3 +++
 4 files changed, 26 insertions(+), 11 deletions(-)
include/exec/cpu-all.h +0 -0
index 0b141683f095..12bd049997ac 100644
@@ -160,7 +160,7 @@ extern int have_guest_base;
 extern unsigned long reserved_va;
Quoted 2 lines
Quoted 2 lines

include/exec/cpu-all.h extern int have_guest_base; · line 163

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

TARGET_VIRT_ADDR_SPACE_BITS) - 1)

Quoted 10 lines
Quoted 3 lines

include/exec/cpu-all.h extern int have_guest_base; · line 163

Quoted 1 line
include/exec/cpu-all.h +1 -0
+                        (2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)) - 1)

I don't understand why you do this change. Could you explain?

For a lot of targets TARGET_VIRT_ADDR_SPACE_BITS is the same as HOST_LONG_BITS, so the shift results in undefined behavior. E.g. without this change I see the following warnings when building such targets for x86_64 host:

linux-user/syscall.c:4905:10: note: in expansion of macro

‘guest_range_valid’

Quoted 10 lines
     if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {
          ^~~~~~~~~~~~~~~~~
include/exec/cpu-all.h:163:30: warning: left shift count >= width of
type [-Wshift-count-overflow]
                         (1ul << (TARGET_VIRT_ADDR_SPACE_BITS)) - 1)

Changing it to 2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1) fixes undefined behavior. Probably that deserves a comment in the changelog.

Perhaps you can keep the scheme used originally with h2g_valid(), it's easier to read:

#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

Thanks,
Laurent

author
#4 lore raw
Quoted 31 lines · Laurent Vivier
Quoted 4 lines · Max Filippov
Quoted 3 lines · Laurent Vivier
Quoted 2 lines · Max Filippov

#define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
- (1ul <<

TARGET_VIRT_ADDR_SPACE_BITS) - 1)

Quoted 10 lines
Quoted 3 lines
Quoted 1 line

+ (2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)) - 1)

I don't understand why you do this change. Could you explain?

For a lot of targets TARGET_VIRT_ADDR_SPACE_BITS is the same as HOST_LONG_BITS, so the shift results in undefined behavior. E.g. without this change I see the following warnings when building such targets for x86_64 host:

linux-user/syscall.c:4905:10: note: in expansion of macro

‘guest_range_valid’

Quoted 10 lines
     if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {
          ^~~~~~~~~~~~~~~~~
include/exec/cpu-all.h:163:30: warning: left shift count >= width of
type [-Wshift-count-overflow]
                         (1ul << (TARGET_VIRT_ADDR_SPACE_BITS)) - 1)

Changing it to 2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1) fixes undefined behavior. Probably that deserves a comment in the changelog.

Perhaps you can keep the scheme used originally with h2g_valid(), it's easier to read:

Quoted 6 lines

#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

Ok, will do.

Signature · 2 lines
Thanks.
-- Max