Re: [Qemu-devel] [PATCH 1/3] linux-user: Don't use MAP_FIXED in do_brk()

[Peter Maydell]

On 18 April 2011 16:34, Peter Maydell peter.mayd...@linaro.org wrote:
 Since mmap() with MAP_FIXED will map over the top of existing mappings,
 it's a bad idea to use it to implement brk(), because brk() with a
 large size is likely to overwrite important things like qemu itself
 or the host libc. So we drop MAP_FIXED and handle mapped but at
 different address as an error case instead.

I've had a report from Martin Mohring that this patch breaks some
programs which previously worked in linux-user mode (one wonders if
they were overwriting some chunk of memory that they happened not
to be using for anything important...)

Anyway, probably better to hold off on applying this patch pending
further investigation. Patches 2/3 and 3/3 in this set should still
be fine to apply though.

-- PMM

[Qemu-devel] [PATCH 1/3] linux-user: Don't use MAP_FIXED in do_brk()

[Peter Maydell]

Since mmap() with MAP_FIXED will map over the top of existing mappings,
it's a bad idea to use it to implement brk(), because brk() with a
large size is likely to overwrite important things like qemu itself
or the host libc. So we drop MAP_FIXED and handle mapped but at
different address as an error case instead.

Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
 linux-user/syscall.c |   29 -
 1 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index bb0999d..e68c5e0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -733,23 +733,34 @@ abi_long do_brk(abi_ulong new_brk)
return target_brk;
 }
 
-/* We need to allocate more memory after the brk... */
+/* We need to allocate more memory after the brk... Note that
+ * we don't use MAP_FIXED because that will map over the top of
+ * any existing mapping (like the one with the host libc or qemu
+ * itself); instead we treat mapped but at wrong address as
+ * a failure and unmap again.
+ */
 new_alloc_size = HOST_PAGE_ALIGN(new_brk - brk_page + 1);
 mapped_addr = get_errno(target_mmap(brk_page, new_alloc_size,
 PROT_READ|PROT_WRITE,
-MAP_ANON|MAP_FIXED|MAP_PRIVATE, 0, 0));
+MAP_ANON|MAP_PRIVATE, 0, 0));
+
+if (mapped_addr == brk_page) {
+target_brk = new_brk;
+return target_brk;
+} else if (mapped_addr != -1) {
+/* Mapped but at wrong address, meaning there wasn't actually
+ * enough space for this brk.
+ */
+target_munmap(mapped_addr, new_alloc_size);
+mapped_addr = -1;
+}
 
 #if defined(TARGET_ALPHA)
 /* We (partially) emulate OSF/1 on Alpha, which requires we
return a proper errno, not an unchanged brk value.  */
-if (is_error(mapped_addr)) {
-return -TARGET_ENOMEM;
-}
+return -TARGET_ENOMEM;
 #endif
-
-if (!is_error(mapped_addr)) {
-   target_brk = new_brk;
-}
+/* For everything else, return the previous break. */
 return target_brk;
 }
 
-- 
1.7.1