[PATCH for 5.0 v2 00/10] A selection of sanitiser fixes

[PATCH for 5.0 v2 00/10] A selection of sanitiser fixes

[Alex Bennée]

Hi,

Here is version 2 of my random fixes series. I've swapped out my fix
to softfloat for Richard's better targeted fix. I also did a re-factor
to init_guest_space so we can use the sanitizer builds on more of the
guests. Initial testing is showing up a few more issues but I didn't
want to hold up review of the changes to date so I can put them in a
PR this week.

The following need review:

 - linux-user: completely re-write init_guest_space
 - linux-user: clean-up padding on /proc/self/maps
 - linux-user: factor out reading of /proc/self/maps
 - target/xtensa: add FIXME for translation memory leak

Alex Bennée (8):
  elf-ops: bail out if we have no function symbols
  linux-user: protect fcntl64 with an #ifdef
  tests/tcg: remove extraneous pasting macros
  linux-user: more debug for init_guest_space
  target/xtensa: add FIXME for translation memory leak
  linux-user: factor out reading of /proc/self/maps
  linux-user: clean-up padding on /proc/self/maps
  linux-user: completely re-write init_guest_space

Denis Plotnikov (1):
  gdbstub: fix compiler complaining

Richard Henderson (1):
  softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

 include/hw/elf_ops.h           |   7 +-
 include/qemu/selfmap.h         |  44 ++++++
 fpu/softfloat.c                |   3 +
 gdbstub.c                      |   4 +-
 linux-user/elfload.c           | 273 +++++++++++++++------------------
 linux-user/syscall.c           |  80 +++++-----
 target/xtensa/translate.c      |   5 +
 util/selfmap.c                 |  74 +++++++++
 tests/tcg/x86_64/system/boot.S |   5 +-
 util/Makefile.objs             |   1 +
 10 files changed, 303 insertions(+), 193 deletions(-)
 create mode 100644 include/qemu/selfmap.h
 create mode 100644 util/selfmap.c

-- 
2.20.1

[PATCH v2 01/10] elf-ops: bail out if we have no function symbols

[Alex Bennée]

It's perfectly possible to have no function symbols in your elf file
and if we do the undefined behaviour sanitizer rightly complains about
us passing NULL to qsort. Check nsyms before we go ahead.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/hw/elf_ops.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index a1411bfcab6..b5d4074d1e3 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -170,8 +170,13 @@ static int glue(load_symbols, SZ)(struct elfhdr *ehdr, int fd, int must_swab,
         }
         i++;
     }
-    syms = g_realloc(syms, nsyms * sizeof(*syms));
 
+    /* check we have symbols left */
+    if (nsyms == 0) {
+        goto fail;
+    }
+
+    syms = g_realloc(syms, nsyms * sizeof(*syms));
     qsort(syms, nsyms, sizeof(*syms), glue(symcmp, SZ));
     for (i = 0; i < nsyms - 1; i++) {
         if (syms[i].st_size == 0) {
-- 
2.20.1

[PATCH v2 02/10] linux-user: protect fcntl64 with an #ifdef

[Alex Bennée]

Checking TARGET_ABI_BITS is sketchy - we should check for the presence
of the define to be sure. Also clean up the white space while we are
there.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5af55fca781..b679bc6b136 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -11331,11 +11331,11 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
            This is a hint, so ignoring and returning success is ok.  */
         return 0;
 #endif
-#if TARGET_ABI_BITS == 32
+#ifdef TARGET_NR_fcntl64
     case TARGET_NR_fcntl64:
     {
-	int cmd;
-	struct flock64 fl;
+        int cmd;
+        struct flock64 fl;
         from_flock64_fn *copyfrom = copy_from_user_flock64;
         to_flock64_fn *copyto = copy_to_user_flock64;
 
@@ -11346,7 +11346,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
         }
 #endif
 
-	cmd = target_to_host_fcntl_cmd(arg2);
+        cmd = target_to_host_fcntl_cmd(arg2);
         if (cmd == -TARGET_EINVAL) {
             return cmd;
         }
-- 
2.20.1

[PATCH v2 03/10] tests/tcg: remove extraneous pasting macros

[Alex Bennée]

We are not using them and they just get in the way.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/tcg/x86_64/system/boot.S | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/tests/tcg/x86_64/system/boot.S b/tests/tcg/x86_64/system/boot.S
index 205cfbd3982..73b19a2bda6 100644
--- a/tests/tcg/x86_64/system/boot.S
+++ b/tests/tcg/x86_64/system/boot.S
@@ -41,10 +41,7 @@
 #define XEN_ELFNOTE_PHYS32_ENTRY  18
 
 #define __ASM_FORM(x)	x
-#define __ASM_FORM_RAW(x)     x
-#define __ASM_FORM_COMMA(x) x,
-#define __ASM_SEL(a,b)           __ASM_FORM(b)
-#define __ASM_SEL_RAW(a,b)      __ASM_FORM_RAW(b)
+#define __ASM_SEL(a,b)  __ASM_FORM(b)
 #define _ASM_PTR	__ASM_SEL(.long, .quad)
 
 	ELFNOTE(Xen, XEN_ELFNOTE_VIRT_BASE,      _ASM_PTR 0x100000)
-- 
2.20.1

[PATCH v2 04/10] linux-user: more debug for init_guest_space

[Alex Bennée]

Searching for memory space can cause problems so lets extend the
CPU_LOG_PAGE output so you can watch init_guest_space fail to
allocate memory. A more involved fix is actually required to make this
function play nicely with the large guard pages the sanitiser likes to
use.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/elfload.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 8198be04460..619c054cc48 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2172,6 +2172,8 @@ unsigned long init_guest_space(unsigned long host_start,
 
         /* Check to see if the address is valid.  */
         if (host_start && real_start != current_start) {
+            qemu_log_mask(CPU_LOG_PAGE, "invalid %lx && %lx != %lx\n",
+                          host_start, real_start, current_start);
             goto try_again;
         }
 
@@ -2240,7 +2242,11 @@ unsigned long init_guest_space(unsigned long host_start,
          * probably a bad strategy if not, which means we got here
          * because of trouble with ARM commpage setup.
          */
-        munmap((void *)real_start, real_size);
+        if (munmap((void *)real_start, real_size) != 0) {
+            error_report("%s: failed to unmap %lx:%lx (%s)", __func__,
+                         real_start, real_size, strerror(errno));
+            abort();
+        }
         current_start += align;
         if (host_start == current_start) {
             /* Theoretically possible if host doesn't have any suitably
-- 
2.20.1

[PATCH v2 05/10] target/xtensa: add FIXME for translation memory leak

[Alex Bennée]

Dynamically allocating a new structure within the DisasContext can
potentially leak as we can longjmp out of the translation loop (see
test_phys_mem). The proper fix would be to use static allocation
within the DisasContext but as the Xtensa translator imports it's code
from elsewhere I leave that as an exercise for the maintainer.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Cc: Max Filippov <jcmvbkbc@gmail.com>
---
 target/xtensa/translate.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
index 8aa972cafdf..37f65b1f030 100644
--- a/target/xtensa/translate.c
+++ b/target/xtensa/translate.c
@@ -1174,6 +1174,11 @@ static void xtensa_tr_init_disas_context(DisasContextBase *dcbase,
     dc->callinc = ((tb_flags & XTENSA_TBFLAG_CALLINC_MASK) >>
                    XTENSA_TBFLAG_CALLINC_SHIFT);
 
+    /*
+     * FIXME: This will leak when a failed instruction load or similar
+     * event causes us to longjump out of the translation loop and
+     * hence not clean-up in xtensa_tr_tb_stop
+     */
     if (dc->config->isa) {
         dc->insnbuf = xtensa_insnbuf_alloc(dc->config->isa);
         dc->slotbuf = xtensa_insnbuf_alloc(dc->config->isa);
-- 
2.20.1

Re: [PATCH v2 05/10] target/xtensa: add FIXME for translation memory leak

[Max Filippov]

On Wed, Apr 1, 2020 at 2:48 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Dynamically allocating a new structure within the DisasContext can
> potentially leak as we can longjmp out of the translation loop (see
> test_phys_mem). The proper fix would be to use static allocation
> within the DisasContext but as the Xtensa translator imports it's code
> from elsewhere I leave that as an exercise for the maintainer.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  target/xtensa/translate.c | 5 +++++
>  1 file changed, 5 insertions(+)

Acked-by: Max Filippov <jcmvbkbc@gmail.com>

-- 
Thanks.
-- Max

[PATCH v2 06/10] gdbstub: fix compiler complaining

[Alex Bennée]

From: Denis Plotnikov <dplotnikov@virtuozzo.com>

    ./gdbstub.c: In function ‘handle_query_thread_extra’:
        /usr/include/glib-2.0/glib/glib-autocleanups.h:28:10:
    error: ‘cpu_name’ may be used uninitialized in this function
    [-Werror=maybe-uninitialized]
        g_free (*pp);
               ^
    ./gdbstub.c:2063:26: note: ‘cpu_name’ was declared here
        g_autofree char *cpu_name;
                         ^
    cc1: all warnings being treated as errors

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
Message-Id: <20200326151407.25046-1-dplotnikov@virtuozzo.com>
Reported-by: Euler Robot <euler.robot@huawei.com>
Reported-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 gdbstub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdbstub.c b/gdbstub.c
index 013fb1ac0f1..171e1509509 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2060,8 +2060,8 @@ static void handle_query_thread_extra(GdbCmdContext *gdb_ctx, void *user_ctx)
         /* Print the CPU model and name in multiprocess mode */
         ObjectClass *oc = object_get_class(OBJECT(cpu));
         const char *cpu_model = object_class_get_name(oc);
-        g_autofree char *cpu_name;
-        cpu_name  = object_get_canonical_path_component(OBJECT(cpu));
+        g_autofree char *cpu_name =
+            object_get_canonical_path_component(OBJECT(cpu));
         g_string_printf(rs, "%s %s [%s]", cpu_model, cpu_name,
                         cpu->halted ? "halted " : "running");
     } else {
-- 
2.20.1

[PATCH v2 07/10] softfloat: Fix BAD_SHIFT from normalizeFloatx80Subnormal

[Alex Bennée]

From: Richard Henderson <richard.henderson@linaro.org>

All other calls to normalize*Subnormal detect zero input before
the call -- this is the only outlier.  This case can happen with
+0.0 + +0.0 = +0.0 or -0.0 + -0.0 = -0.0, so return a zero of
the correct sign.

Reported-by: Coverity (CID 1421991)
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20200327232042.10008-1-richard.henderson@linaro.org>
---
 fpu/softfloat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fpu/softfloat.c b/fpu/softfloat.c
index 301ce3b537b..ae6ba718540 100644
--- a/fpu/softfloat.c
+++ b/fpu/softfloat.c
@@ -5856,6 +5856,9 @@ static floatx80 addFloatx80Sigs(floatx80 a, floatx80 b, flag zSign,
         zSig1 = 0;
         zSig0 = aSig + bSig;
         if ( aExp == 0 ) {
+            if (zSig0 == 0) {
+                return packFloatx80(zSign, 0, 0);
+            }
             normalizeFloatx80Subnormal( zSig0, &zExp, &zSig0 );
             goto roundAndPack;
         }
-- 
2.20.1

[PATCH v2 08/10] linux-user: factor out reading of /proc/self/maps

[Alex Bennée]

Unfortunately reading /proc/self/maps is still considered the gold
standard for a process finding out about it's own memory layout. As we
will want this data in other contexts soon factor out the code to read
and parse the data. Rather than just blindly copying the existing
sscanf based code we use a more modern glib version of the parsing
code to make a more general purpose map structure.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/qemu/selfmap.h | 44 +++++++++++++++++++++++++
 linux-user/syscall.c   | 58 ++++++++++++++++-----------------
 util/selfmap.c         | 74 ++++++++++++++++++++++++++++++++++++++++++
 util/Makefile.objs     |  1 +
 4 files changed, 147 insertions(+), 30 deletions(-)
 create mode 100644 include/qemu/selfmap.h
 create mode 100644 util/selfmap.c

diff --git a/include/qemu/selfmap.h b/include/qemu/selfmap.h
new file mode 100644
index 00000000000..3bc96feb055
--- /dev/null
+++ b/include/qemu/selfmap.h
@@ -0,0 +1,44 @@
+/*
+ * Utility functions to read our own memory map
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef _SELFMAP_H_
+#define _SELFMAP_H_
+
+typedef struct {
+    uint64_t start;
+    uint64_t end;
+
+    /* flags */
+    bool is_read;
+    bool is_write;
+    bool is_exec;
+    bool is_priv;
+
+    uint64_t offset;
+    gchar *dev;
+    int   inode;
+    gchar *path;
+} MapInfo;
+
+
+/**
+ * read_self_maps:
+ *
+ * Read /proc/self/maps and return a list of MapInfo structures.
+ */
+GSList *read_self_maps(void);
+
+/**
+ * free_self_maps:
+ * @info: a GSlist
+ *
+ * Free a list of MapInfo structures.
+ */
+void free_self_maps(GSList *info);
+
+#endif /* _SELFMAP_H_ */
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b679bc6b136..0246df01573 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -117,6 +117,7 @@
 
 #include "qemu.h"
 #include "qemu/guest-random.h"
+#include "qemu/selfmap.h"
 #include "user/syscall-trace.h"
 #include "qapi/error.h"
 #include "fd-trans.h"
@@ -7232,45 +7233,45 @@ static int open_self_maps(void *cpu_env, int fd)
 {
     CPUState *cpu = env_cpu((CPUArchState *)cpu_env);
     TaskState *ts = cpu->opaque;
-    FILE *fp;
-    char *line = NULL;
-    size_t len = 0;
-    ssize_t read;
+    GSList *map_info = read_self_maps();
+    GSList *s;
 
-    fp = fopen("/proc/self/maps", "r");
-    if (fp == NULL) {
-        return -1;
-    }
+    for (s = map_info; s; s = g_slist_next(s)) {
+        MapInfo *e = (MapInfo *) s->data;
 
-    while ((read = getline(&line, &len, fp)) != -1) {
-        int fields, dev_maj, dev_min, inode;
-        uint64_t min, max, offset;
-        char flag_r, flag_w, flag_x, flag_p;
-        char path[512] = "";
-        fields = sscanf(line, "%"PRIx64"-%"PRIx64" %c%c%c%c %"PRIx64" %x:%x %d"
-                        " %512s", &min, &max, &flag_r, &flag_w, &flag_x,
-                        &flag_p, &offset, &dev_maj, &dev_min, &inode, path);
-
-        if ((fields < 10) || (fields > 11)) {
-            continue;
-        }
-        if (h2g_valid(min)) {
+        if (h2g_valid(e->start)) {
+            uint64_t min = e->start;
+            uint64_t max = e->end;
             int flags = page_get_flags(h2g(min));
-            max = h2g_valid(max - 1) ? max : (uintptr_t)g2h(GUEST_ADDR_MAX) + 1;
+            const char *path;
+
+            max = h2g_valid(max - 1) ?
+                max : (uintptr_t) g2h(GUEST_ADDR_MAX) + 1;
+
             if (page_check_range(h2g(min), max - min, flags) == -1) {
                 continue;
             }
+
             if (h2g(min) == ts->info->stack_limit) {
-                pstrcpy(path, sizeof(path), "      [stack]");
+                path = "      [stack]";
+            } else {
+                path = e->path;
             }
+
             dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr
-                    " %c%c%c%c %08" PRIx64 " %02x:%02x %d %s%s\n",
-                    h2g(min), h2g(max - 1) + 1, flag_r, flag_w,
-                    flag_x, flag_p, offset, dev_maj, dev_min, inode,
-                    path[0] ? "         " : "", path);
+                    " %c%c%c%c %08" PRIx64 " %s %d %s%s\n",
+                    h2g(min), h2g(max - 1) + 1,
+                    e->is_read ? 'r' : '-',
+                    e->is_write ? 'w' : '-',
+                    e->is_exec ? 'x' : '-',
+                    e->is_priv ? 'p' : '-',
+                    e->offset, e->dev, e->inode,
+                    path ? "         " : "", path ? path : "");
         }
     }
 
+    free_self_maps(map_info);
+
 #ifdef TARGET_VSYSCALL_PAGE
     /*
      * We only support execution from the vsyscall page.
@@ -7281,9 +7282,6 @@ static int open_self_maps(void *cpu_env, int fd)
             TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE);
 #endif
 
-    free(line);
-    fclose(fp);
-
     return 0;
 }
 
diff --git a/util/selfmap.c b/util/selfmap.c
new file mode 100644
index 00000000000..d72b2c32f07
--- /dev/null
+++ b/util/selfmap.c
@@ -0,0 +1,74 @@
+/*
+ * Utility function to get QEMU's own process map
+ *
+ * Copyright (c) 2020 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/cutils.h"
+#include "qemu/selfmap.h"
+
+GSList *read_self_maps(void)
+{
+    gchar *maps;
+    GSList *map_info = NULL;
+
+    if (g_file_get_contents("/proc/self/maps", &maps, NULL, NULL)) {
+        gchar **lines = g_strsplit(maps, "\n", 0);
+        int i, entries = g_strv_length(lines);
+
+        for (i = 0; i < entries; i++) {
+            gchar **fields = g_strsplit(lines[i], " ", 0);
+            if (g_strv_length(fields) > 4) {
+                MapInfo *e = g_new0(MapInfo, 1);
+                int errors;
+                const char *end;
+
+                errors  = qemu_strtoul(fields[0], &end, 16, &e->start);
+                errors += qemu_strtoul(end + 1, NULL, 16, &e->end);
+
+                e->is_read  = fields[1][0] == 'r' ? true : false;
+                e->is_write = fields[1][1] == 'w' ? true : false;
+                e->is_exec  = fields[1][2] == 'x' ? true : false;
+                e->is_priv  = fields[1][3] == 'p' ? true : false;
+
+                errors += qemu_strtoul(fields[2], NULL, 16, &e->offset);
+                e->dev = g_strdup(fields[3]);
+                errors += qemu_strtoi(fields[4], NULL, 10, &e->inode);
+
+                /* A bit ugly as strsplit doesn't skip multiple separators */
+                if (g_strv_length(fields) > 6) {
+                    e->path = g_strdup(fields[g_strv_length(fields) - 1]);
+                }
+                map_info = g_slist_prepend(map_info, e);
+            }
+
+            g_strfreev(fields);
+        }
+        g_strfreev(lines);
+        g_free(maps);
+    }
+
+    /* ensure the map data is in the same order we collected it */
+    return g_slist_reverse(map_info);
+}
+
+/**
+ * free_self_maps:
+ * @info: a GSlist
+ *
+ * Free a list of MapInfo structures.
+ */
+static void free_info(gpointer data)
+{
+    MapInfo *e = (MapInfo *) data;
+    g_free(e->dev);
+    g_free(e->path);
+}
+
+void free_self_maps(GSList *info)
+{
+    g_slist_free_full(info, &free_info);
+}
diff --git a/util/Makefile.objs b/util/Makefile.objs
index 6718a38b616..fe339c2636b 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -63,3 +63,4 @@ util-obj-y += guest-random.o
 util-obj-$(CONFIG_GIO) += dbus.o
 dbus.o-cflags = $(GIO_CFLAGS)
 dbus.o-libs = $(GIO_LIBS)
+util-obj-$(CONFIG_USER_ONLY) += selfmap.o
-- 
2.20.1

Re: [PATCH v2 08/10] linux-user: factor out reading of /proc/self/maps

[Richard Henderson]

On 4/1/20 2:47 AM, Alex Bennée wrote:
> +typedef struct {
> +    uint64_t start;
> +    uint64_t end;
...
> +                errors  = qemu_strtoul(fields[0], &end, 16, &e->start);
> +                errors += qemu_strtoul(end + 1, NULL, 16, &e->end);

uint64_t vs unsigned long -- you want qemu_strtou64.

> +                errors += qemu_strtoul(fields[2], NULL, 16, &e->offset);

Likewise.

> +                /* A bit ugly as strsplit doesn't skip multiple separators */
> +                if (g_strv_length(fields) > 6) {
> +                    e->path = g_strdup(fields[g_strv_length(fields) - 1]);
> +                }

And if the path contains spaces?


r~

Re: [PATCH v2 08/10] linux-user: factor out reading of /proc/self/maps

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 4/1/20 2:47 AM, Alex Bennée wrote:
>> +typedef struct {
>> +    uint64_t start;
>> +    uint64_t end;
> ...
>> +                errors  = qemu_strtoul(fields[0], &end, 16, &e->start);
>> +                errors += qemu_strtoul(end + 1, NULL, 16, &e->end);
>
> uint64_t vs unsigned long -- you want qemu_strtou64.
>
>> +                errors += qemu_strtoul(fields[2], NULL, 16, &e->offset);
>
> Likewise.

Actually I went to using unsigned longs in the structure as that is the
natural size for host map info.

>
>> +                /* A bit ugly as strsplit doesn't skip multiple separators */
>> +                if (g_strv_length(fields) > 6) {
>> +                    e->path = g_strdup(fields[g_strv_length(fields) - 1]);
>> +                }
>
> And if the path contains spaces?

interesting bugs I guess - I'll see if I can do a cleaner pass.
>
>
> r~


-- 
Alex Bennée

[PATCH v2 09/10] linux-user: clean-up padding on /proc/self/maps

[Alex Bennée]

Don't use magic spaces, calculate the justification for the file
field like the kernel does with seq_pad.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/syscall.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0246df01573..b921432f4ff 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7235,6 +7235,7 @@ static int open_self_maps(void *cpu_env, int fd)
     TaskState *ts = cpu->opaque;
     GSList *map_info = read_self_maps();
     GSList *s;
+    int count;
 
     for (s = map_info; s; s = g_slist_next(s)) {
         MapInfo *e = (MapInfo *) s->data;
@@ -7253,20 +7254,24 @@ static int open_self_maps(void *cpu_env, int fd)
             }
 
             if (h2g(min) == ts->info->stack_limit) {
-                path = "      [stack]";
+                path = "[stack]";
             } else {
                 path = e->path;
             }
 
-            dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr
-                    " %c%c%c%c %08" PRIx64 " %s %d %s%s\n",
-                    h2g(min), h2g(max - 1) + 1,
-                    e->is_read ? 'r' : '-',
-                    e->is_write ? 'w' : '-',
-                    e->is_exec ? 'x' : '-',
-                    e->is_priv ? 'p' : '-',
-                    e->offset, e->dev, e->inode,
-                    path ? "         " : "", path ? path : "");
+            count = dprintf(fd, TARGET_ABI_FMT_ptr "-" TARGET_ABI_FMT_ptr
+                            " %c%c%c%c %08" PRIx64 " %s %d",
+                            h2g(min), h2g(max - 1) + 1,
+                            e->is_read ? 'r' : '-',
+                            e->is_write ? 'w' : '-',
+                            e->is_exec ? 'x' : '-',
+                            e->is_priv ? 'p' : '-',
+                            e->offset, e->dev, e->inode);
+            if (path) {
+                dprintf(fd, "%*s%s\n", 73 - count, "", path);
+            } else {
+                dprintf(fd, "\n");
+            }
         }
     }
 
@@ -7277,9 +7282,10 @@ static int open_self_maps(void *cpu_env, int fd)
      * We only support execution from the vsyscall page.
      * This is as if CONFIG_LEGACY_VSYSCALL_XONLY=y from v5.3.
      */
-    dprintf(fd, TARGET_FMT_lx "-" TARGET_FMT_lx
-            " --xp 00000000 00:00 0 [vsyscall]\n",
-            TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE);
+    count = dprintf(fd, TARGET_FMT_lx "-" TARGET_FMT_lx
+                    " --xp 00000000 00:00 0",
+                    TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE);
+    dprintf(fd, "%*s%s\n", 73 - count, "",  "[vsyscall]");
 #endif
 
     return 0;
-- 
2.20.1

Re: [PATCH v2 09/10] linux-user: clean-up padding on /proc/self/maps

[Richard Henderson]

On 4/1/20 2:47 AM, Alex Bennée wrote:
> Don't use magic spaces, calculate the justification for the file
> field like the kernel does with seq_pad.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  linux-user/syscall.c | 32 +++++++++++++++++++-------------
>  1 file changed, 19 insertions(+), 13 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH v2 10/10] linux-user: completely re-write init_guest_space

[Alex Bennée]

This tries to simplify the init_guest_space code to be a little less
convoluted and remove the brute force mapping algorithm that gets
tripped up so badly by the sanitizers.

We first try to do what is requested by the host. Failing that we try
and satisfy the guest requested base address. If all those options
fail we fall back to finding a space in the memory map using our
recently written read_self_maps() helper.

Less mind-binding gotos and hopefully clearer logic although perhaps
more sloppy casting than I'm totally happy with.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/elfload.c | 279 +++++++++++++++++++------------------------
 1 file changed, 125 insertions(+), 154 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 619c054cc48..88c08513119 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -11,6 +11,7 @@
 #include "qemu/queue.h"
 #include "qemu/guest-random.h"
 #include "qemu/units.h"
+#include "qemu/selfmap.h"
 
 #ifdef _ARCH_PPC64
 #undef ARCH_DLINFO
@@ -2075,6 +2076,34 @@ static abi_ulong create_elf_tables(abi_ulong p, int argc, int envc,
     return sp;
 }
 
+/*
+ * Wrapper to hide to keep the ugliness of the commpage checks out of
+ * the init_guest_space function bellow. For non-32 bit ARM targets it
+ * always succeeds.
+ */
+static bool check_commpage(unsigned long start, unsigned long size)
+{
+#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
+    if (init_guest_commpage(start, size) != 1) {
+        return false;
+    }
+#endif
+   return true;
+}
+
+/*
+ * init_guest_space:
+ *
+ * Reserve the initial chunk of guest address space. In order we try:
+ *
+ *   - if given host_start just verify it
+ *   - else try and allocate at guest_start to save offset calculations
+ *   - finally allocate from lowest available >= host_size'd gap
+ *
+ * In practice it shouldn't matter if the guest can't extend brk above
+ * it's initial allocation because any moderately sane memory
+ * allocation library should be using mmap to allocate additional blocks.
+ */
 unsigned long init_guest_space(unsigned long host_start,
                                unsigned long host_size,
                                unsigned long guest_start,
@@ -2082,183 +2111,125 @@ unsigned long init_guest_space(unsigned long host_start,
 {
     /* In order to use host shmat, we must be able to honor SHMLBA.  */
     unsigned long align = MAX(SHMLBA, qemu_host_page_size);
-    unsigned long current_start, aligned_start;
-    int flags;
+    void *map_addr = NULL;
+    const int flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE | MAP_FIXED;
 
     assert(host_start || host_size);
 
-    /* If just a starting address is given, then just verify that
-     * address.  */
+    /*
+     * If just a starting address is given, then just verify that
+     * address. If the commpage isn't happy we pretty much give up
+     * now.
+     */
     if (host_start && !host_size) {
-#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-        if (init_guest_commpage(host_start, host_size) != 1) {
+        if (!check_commpage(host_start, host_size)) {
             return (unsigned long)-1;
+        } else {
+            qemu_log_mask(CPU_LOG_PAGE, "%s: host_start @ %#lx verified\n",
+                          __func__, host_start);
+            return host_start;
         }
-#endif
-        return host_start;
     }
 
-    /* Setup the initial flags and start address.  */
-    current_start = host_start & -align;
-    flags = MAP_ANONYMOUS | MAP_PRIVATE | MAP_NORESERVE;
-    if (fixed) {
-        flags |= MAP_FIXED;
-    }
-
-    /* Otherwise, a non-zero size region of memory needs to be mapped
-     * and validated.  */
-
-#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-    /* On 32-bit ARM, we need to map not just the usable memory, but
-     * also the commpage.  Try to find a suitable place by allocating
-     * a big chunk for all of it.  If host_start, then the naive
-     * strategy probably does good enough.
+    /*
+     * Now we are going to try and map something, we start by trying
+     * to satisfy exactly what the guest wants. This is unlikely to
+     * succeed but will make the code generators job easier if it can
+     * be done.
+     *
+     * If the commpage check isn't happy after we allocate we need to
+     * fall back to finding a big enough hole in the address space.
      */
-    if (!host_start) {
-        unsigned long guest_full_size, host_full_size, real_start;
-
-        guest_full_size =
-            (0xffff0f00 & qemu_host_page_mask) + qemu_host_page_size;
-        host_full_size = guest_full_size - guest_start;
-        real_start = (unsigned long)
-            mmap(NULL, host_full_size, PROT_NONE, flags, -1, 0);
-        if (real_start == (unsigned long)-1) {
-            if (host_size < host_full_size - qemu_host_page_size) {
-                /* We failed to map a continous segment, but we're
-                 * allowed to have a gap between the usable memory and
-                 * the commpage where other things can be mapped.
-                 * This sparseness gives us more flexibility to find
-                 * an address range.
-                 */
-                goto naive;
-            }
-            return (unsigned long)-1;
+    map_addr = (void *) guest_start;
+    if (mmap(map_addr, host_size, PROT_NONE, flags, -1, 0) == map_addr) {
+        if (check_commpage(guest_start, host_size)) {
+            /* success, everyone is happy :-D */
+            qemu_log_mask(CPU_LOG_PAGE, "%s: got what guest wanted @ %p\n",
+                          __func__, map_addr);
+            return guest_start;
         }
-        munmap((void *)real_start, host_full_size);
-        if (real_start & (align - 1)) {
-            /* The same thing again, but with extra
-             * so that we can shift around alignment.
-             */
-            unsigned long real_size = host_full_size + qemu_host_page_size;
-            real_start = (unsigned long)
-                mmap(NULL, real_size, PROT_NONE, flags, -1, 0);
-            if (real_start == (unsigned long)-1) {
-                if (host_size < host_full_size - qemu_host_page_size) {
-                    goto naive;
-                }
-                return (unsigned long)-1;
-            }
-            munmap((void *)real_start, real_size);
-            real_start = ROUND_UP(real_start, align);
-        }
-        current_start = real_start;
-    }
- naive:
-#endif
-
-    while (1) {
-        unsigned long real_start, real_size, aligned_size;
-        aligned_size = real_size = host_size;
 
-        /* Do not use mmap_find_vma here because that is limited to the
-         * guest address space.  We are going to make the
-         * guest address space fit whatever we're given.
-         */
-        real_start = (unsigned long)
-            mmap((void *)current_start, host_size, PROT_NONE, flags, -1, 0);
-        if (real_start == (unsigned long)-1) {
-            return (unsigned long)-1;
-        }
-
-        /* Check to see if the address is valid.  */
-        if (host_start && real_start != current_start) {
-            qemu_log_mask(CPU_LOG_PAGE, "invalid %lx && %lx != %lx\n",
-                          host_start, real_start, current_start);
-            goto try_again;
+        if (munmap(map_addr, host_size) != 0) {
+            error_report("%s: failed to unmap %p:%lx (%s)", __func__,
+                         map_addr, host_size, strerror(errno));
+            abort();
         }
+    } else if (fixed) {
+        /*
+         * If the caller wanted a fixed address we have pretty much failed
+         * to deliver here so it is time to bail out gracefully.
+         */
+        error_report("%s: failed to honour fixed guest request @ %p",
+                     __func__, map_addr);
+        return (unsigned long)-1;
+    }
 
-        /* Ensure the address is properly aligned.  */
-        if (real_start & (align - 1)) {
-            /* Ideally, we adjust like
-             *
-             *    pages: [  ][  ][  ][  ][  ]
-             *      old:   [   real   ]
-             *             [ aligned  ]
-             *      new:   [     real     ]
-             *               [ aligned  ]
-             *
-             * But if there is something else mapped right after it,
-             * then obviously it won't have room to grow, and the
-             * kernel will put the new larger real someplace else with
-             * unknown alignment (if we made it to here, then
-             * fixed=false).  Which is why we grow real by a full page
-             * size, instead of by part of one; so that even if we get
-             * moved, we can still guarantee alignment.  But this does
-             * mean that there is a padding of < 1 page both before
-             * and after the aligned range; the "after" could could
-             * cause problems for ARM emulation where it could butt in
-             * to where we need to put the commpage.
-             */
-            munmap((void *)real_start, host_size);
-            real_size = aligned_size + align;
-            real_start = (unsigned long)
-                mmap((void *)real_start, real_size, PROT_NONE, flags, -1, 0);
-            if (real_start == (unsigned long)-1) {
-                return (unsigned long)-1;
+    /*
+     * Finally we need to find a hole somewhere in the address space
+     * that will accept the initial mapping as well as being able to
+     * map the (ARM32 specific) commpage later.
+     *
+     * We need to ensure the address is properly aligned. But this
+     * does mean that there is a padding of < 1 page both before and
+     * after the aligned range; the "after" could could cause problems
+     * for aforementioned ARM32 emulation.
+     */
+    {
+#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
+        uint64_t required_size =
+            (0xffff0f00 & qemu_host_page_mask) + qemu_host_page_size;
+#else
+        uint64_t required_size = host_size + align;
+#endif
+        GSList *map_info = read_self_maps();
+        GSList *last, *next;
+        map_addr = NULL;
+
+        for (last = map_info, next = g_slist_next(last);
+             next; last = next, next = g_slist_next(next)) {
+            MapInfo *l = (MapInfo *) last->data;
+            MapInfo *n = (MapInfo *) next->data;
+            uint64_t base = ROUND_UP(l->end, align);
+            uint64_t gap_size = n->start - base;
+            if (gap_size > required_size) {
+                map_addr = (void *) base;
+                break;
             }
-            aligned_start = ROUND_UP(real_start, align);
-        } else {
-            aligned_start = real_start;
         }
 
-#if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
-        /* On 32-bit ARM, we need to also be able to map the commpage.  */
-        int valid = init_guest_commpage(aligned_start - guest_start,
-                                        aligned_size + guest_start);
-        if (valid == -1) {
-            munmap((void *)real_start, real_size);
+        /*
+         * We couldn't find any space in the memory map, woe...
+         */
+        if (!map_addr) {
+            error_report("%s: couldn't find a %ld sized gap in the memory map",
+                         __func__, required_size);
             return (unsigned long)-1;
-        } else if (valid == 0) {
-            goto try_again;
         }
-#endif
+    }
 
-        /* If nothing has said `return -1` or `goto try_again` yet,
-         * then the address we have is good.
-         */
-        break;
-
-    try_again:
-        /* That address didn't work.  Unmap and try a different one.
-         * The address the host picked because is typically right at
-         * the top of the host address space and leaves the guest with
-         * no usable address space.  Resort to a linear search.  We
-         * already compensated for mmap_min_addr, so this should not
-         * happen often.  Probably means we got unlucky and host
-         * address space randomization put a shared library somewhere
-         * inconvenient.
-         *
-         * This is probably a good strategy if host_start, but is
-         * probably a bad strategy if not, which means we got here
-         * because of trouble with ARM commpage setup.
-         */
-        if (munmap((void *)real_start, real_size) != 0) {
-            error_report("%s: failed to unmap %lx:%lx (%s)", __func__,
-                         real_start, real_size, strerror(errno));
+    /*
+     * From this point on it should be a formality but lets go through
+     * the steps anyway.
+     */
+    if (mmap(map_addr, host_size + align , PROT_NONE,
+             flags | MAP_FIXED, -1, 0) == map_addr) {
+        unsigned long addr = (unsigned long) map_addr;
+        if (!check_commpage(addr, host_size + align)) {
+            error_report("%s: commpage won't fit in guest_memory @ %p",
+                         __func__, map_addr);
             abort();
+        } else {
+            qemu_log_mask(CPU_LOG_PAGE, "%s: guest address space @ %p\n",
+                          __func__, map_addr);
+            return addr;
         }
-        current_start += align;
-        if (host_start == current_start) {
-            /* Theoretically possible if host doesn't have any suitably
-             * aligned areas.  Normally the first mmap will fail.
-             */
-            return (unsigned long)-1;
-        }
+    } else {
+        error_report("%s: failed to allocate guest address space @ %p (%d/%s)",
+                     __func__, map_addr, errno, strerror(errno));
     }
 
-    qemu_log_mask(CPU_LOG_PAGE, "Reserved 0x%lx bytes of guest address space\n", host_size);
-
-    return aligned_start;
+    /* really should never get here */
+    g_assert_not_reached();
 }
 
 static void probe_guest_base(const char *image_name,
-- 
2.20.1

Re: [PATCH v2 10/10] linux-user: completely re-write init_guest_space

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> This tries to simplify the init_guest_space code to be a little less
> convoluted and remove the brute force mapping algorithm that gets
> tripped up so badly by the sanitizers.
>
> We first try to do what is requested by the host. Failing that we try
> and satisfy the guest requested base address. If all those options
> fail we fall back to finding a space in the memory map using our
> recently written read_self_maps() helper.
>
> Less mind-binding gotos and hopefully clearer logic although perhaps
> more sloppy casting than I'm totally happy with.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  linux-user/elfload.c | 279 +++++++++++++++++++------------------------
>  1 file changed, 125 insertions(+), 154 deletions(-)

Richard,

I think I overthought the magic alignment code as we can deal with it
all in advance and not have to add extra pages which might get in the
way:

1 file changed, 5 insertions(+), 7 deletions(-)
linux-user/elfload.c | 12 +++++-------

modified   linux-user/elfload.c
@@ -2169,17 +2169,15 @@ unsigned long init_guest_space(unsigned long host_start,
      * that will accept the initial mapping as well as being able to
      * map the (ARM32 specific) commpage later.
      *
-     * We need to ensure the address is properly aligned. But this
-     * does mean that there is a padding of < 1 page both before and
-     * after the aligned range; the "after" could could cause problems
-     * for aforementioned ARM32 emulation.
+     * We need to ensure the address is properly aligned but we can
+     * take that into account when looking for the gap.
      */
     {
 #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
         uint64_t required_size =
             (0xffff0f00 & qemu_host_page_mask) + qemu_host_page_size;
 #else
-        uint64_t required_size = host_size + align;
+        uint64_t required_size = host_size;
 #endif
         GSList *map_info = read_self_maps();
         GSList *last, *next;
@@ -2211,10 +2209,10 @@ unsigned long init_guest_space(unsigned long host_start,
      * From this point on it should be a formality but lets go through
      * the steps anyway.
      */
-    if (mmap(map_addr, host_size + align , PROT_NONE,
+    if (mmap(map_addr, host_size, PROT_NONE,
              flags | MAP_FIXED, -1, 0) == map_addr) {
         unsigned long addr = (unsigned long) map_addr;
-        if (!check_commpage(addr, host_size + align)) {
+        if (!check_commpage(addr, host_size)) {
             error_report("%s: commpage won't fit in guest_memory @ %p",
                          __func__, map_addr);
             abort();


-- 
Alex Bennée

Re: [PATCH v2 10/10] linux-user: completely re-write init_guest_space

[Richard Henderson]

On 4/1/20 2:47 AM, Alex Bennée wrote:
> +    /*
> +     * Now we are going to try and map something, we start by trying
> +     * to satisfy exactly what the guest wants. This is unlikely to
> +     * succeed but will make the code generators job easier if it can
> +     * be done.
> +     *
> +     * If the commpage check isn't happy after we allocate we need to
> +     * fall back to finding a big enough hole in the address space.
>       */
> +    map_addr = (void *) guest_start;
> +    if (mmap(map_addr, host_size, PROT_NONE, flags, -1, 0) == map_addr) {

Not recording the result of the mmap is wrong.

There are not just two options, as implied by your "== map_addr" check: you are
missing out on the mmap succeeds (!= MAP_FAILED) but still not equal to map_addr.

If the kernel gives us a different address than the one requested, we can
either decide to use it, or unmap it again.  We can't do either with the above.

This is definitely going to have to wait for 5.1.


r~