[PATCH v3 0/8] gdbstub: Add support for info proc mappings

[PATCH v3 0/8] gdbstub: Add support for info proc mappings

[Ilya Leoshkevich]

v2: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg06837.html
v2 -> v3: Use openat() instead of safe_openat() (new patch: 2/8).
          Add /proc/self/smaps emulation (new patch: 3/8).
          With these 2 changes, the minor issues previously mentioned in
          the patch 6/8 are gone.

v1: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg02614.html
v1 -> v2: Reword the 5/6 commit message (Dominik).
          Add R-bs.
          Patches that need review:
          4/6 gdbstub: Add support for info proc mappings
          6/6 tests/tcg: Add a test for info proc mappings

Hi,

this series partially implements the Host I/O feature of the GDB Remote
Serial Protocol in order to make generate-core-file work with qemu-user.
It borrows heavily from the abandoned patch by Dominik [1], hence 4/6
carries the respective Co-developed-by: tag. I hope that's okay. I also
peeked at gdbserver/hostio.cc quite a few times.

The changes compared to Dominik's patch are:

- Implement readlink.
- Move the main functionality to user-target.c.
- Allocate buffers on heap.
- Add a test.
- Update gdb.rst.
- Split refactorings to the existing code into separate patches.
- Rename do_openat() to do_guest_openat().
- Do not retry pread(), since GDB is capable of doing it itself.
- Add an extra sanity check to gdb_handle_query_xfer_exec_file().
- Replace citations of the spec by a single link.

Best regards,
Ilya

Ilya Leoshkevich (8):
  linux-user: Expose do_guest_openat() and do_guest_readlink()
  linux-user: Add "safe" parameter to do_guest_openat()
  linux-user: Emulate /proc/self/smaps
  gdbstub: Expose gdb_get_process() and gdb_get_first_cpu_in_process()
  gdbstub: Report the actual qemu-user pid
  gdbstub: Add support for info proc mappings
  docs: Document security implications of debugging
  tests/tcg: Add a test for info proc mappings

 docs/system/gdb.rst                           |  15 ++
 gdbstub/gdbstub.c                             |  86 ++++++++---
 gdbstub/internals.h                           |   7 +
 gdbstub/user-target.c                         | 139 ++++++++++++++++++
 linux-user/qemu.h                             |   3 +
 linux-user/syscall.c                          | 128 +++++++++++++---
 tests/tcg/aarch64/Makefile.target             |   3 +-
 tests/tcg/multiarch/Makefile.target           |   7 +
 .../multiarch/gdbstub/test-proc-mappings.py   |  55 +++++++
 tests/tcg/s390x/Makefile.target               |   2 +-
 10 files changed, 401 insertions(+), 44 deletions(-)
 create mode 100644 tests/tcg/multiarch/gdbstub/test-proc-mappings.py

-- 
2.40.1

[PATCH v3 1/8] linux-user: Expose do_guest_openat() and do_guest_readlink()

[Ilya Leoshkevich]

These functions will be required by the GDB stub in order to provide
the guest view of /proc to GDB.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/qemu.h    |  3 +++
 linux-user/syscall.c | 54 ++++++++++++++++++++++++++++----------------
 2 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 92f9f5af41c..a5830ec2396 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -165,6 +165,9 @@ typedef struct TaskState {
 } TaskState;
 
 abi_long do_brk(abi_ulong new_brk);
+int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
+                    int flags, mode_t mode);
+ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz);
 
 /* user access */
 
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 83685f0aa59..2d3070cfd62 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8448,7 +8448,8 @@ static int open_hardware(CPUArchState *cpu_env, int fd)
 }
 #endif
 
-static int do_openat(CPUArchState *cpu_env, int dirfd, const char *pathname, int flags, mode_t mode)
+int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
+                    int flags, mode_t mode)
 {
     struct fake_open {
         const char *filename;
@@ -8520,6 +8521,36 @@ static int do_openat(CPUArchState *cpu_env, int dirfd, const char *pathname, int
     return safe_openat(dirfd, path(pathname), flags, mode);
 }
 
+ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz)
+{
+    ssize_t ret;
+
+    if (!pathname || !buf) {
+        errno = EFAULT;
+        return -1;
+    }
+
+    if (!bufsiz) {
+        /* Short circuit this for the magic exe check. */
+        errno = EINVAL;
+        return -1;
+    }
+
+    if (is_proc_myself((const char *)pathname, "exe")) {
+        /*
+         * Don't worry about sign mismatch as earlier mapping
+         * logic would have thrown a bad address error.
+         */
+        ret = MIN(strlen(exec_path), bufsiz);
+        /* We cannot NUL terminate the string. */
+        memcpy(buf, exec_path, ret);
+    } else {
+        ret = readlink(path(pathname), buf, bufsiz);
+    }
+
+    return ret;
+}
+
 static int do_execveat(CPUArchState *cpu_env, int dirfd,
                        abi_long pathname, abi_long guest_argp,
                        abi_long guest_envp, int flags)
@@ -8994,7 +9025,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
     case TARGET_NR_open:
         if (!(p = lock_user_string(arg1)))
             return -TARGET_EFAULT;
-        ret = get_errno(do_openat(cpu_env, AT_FDCWD, p,
+        ret = get_errno(do_guest_openat(cpu_env, AT_FDCWD, p,
                                   target_to_host_bitmask(arg2, fcntl_flags_tbl),
                                   arg3));
         fd_trans_unregister(ret);
@@ -9004,7 +9035,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
     case TARGET_NR_openat:
         if (!(p = lock_user_string(arg2)))
             return -TARGET_EFAULT;
-        ret = get_errno(do_openat(cpu_env, arg1, p,
+        ret = get_errno(do_guest_openat(cpu_env, arg1, p,
                                   target_to_host_bitmask(arg3, fcntl_flags_tbl),
                                   arg4));
         fd_trans_unregister(ret);
@@ -10229,22 +10260,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
             void *p2;
             p = lock_user_string(arg1);
             p2 = lock_user(VERIFY_WRITE, arg2, arg3, 0);
-            if (!p || !p2) {
-                ret = -TARGET_EFAULT;
-            } else if (!arg3) {
-                /* Short circuit this for the magic exe check. */
-                ret = -TARGET_EINVAL;
-            } else if (is_proc_myself((const char *)p, "exe")) {
-                /*
-                 * Don't worry about sign mismatch as earlier mapping
-                 * logic would have thrown a bad address error.
-                 */
-                ret = MIN(strlen(exec_path), arg3);
-                /* We cannot NUL terminate the string. */
-                memcpy(p2, exec_path, ret);
-            } else {
-                ret = get_errno(readlink(path(p), p2, arg3));
-            }
+            ret = get_errno(do_guest_readlink(p, p2, arg3));
             unlock_user(p2, arg2, ret);
             unlock_user(p, arg1, 0);
         }
-- 
2.40.1

[PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat()

[Ilya Leoshkevich]

gdbstub cannot meaningfully handle QEMU_ERESTARTSYS, and it doesn't
need to. Add a parameter to do_guest_openat() that makes it use
openat() instead of safe_openat(), so that it becomes usable from
gdbstub.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/qemu.h    |  2 +-
 linux-user/syscall.c | 18 +++++++++++++-----
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index a5830ec2396..9b8e0860d70 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -166,7 +166,7 @@ typedef struct TaskState {
 
 abi_long do_brk(abi_ulong new_brk);
 int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
-                    int flags, mode_t mode);
+                    int flags, mode_t mode, bool safe);
 ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz);
 
 /* user access */
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 2d3070cfd62..28a0b1f7882 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8449,7 +8449,7 @@ static int open_hardware(CPUArchState *cpu_env, int fd)
 #endif
 
 int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
-                    int flags, mode_t mode)
+                    int flags, mode_t mode, bool safe)
 {
     struct fake_open {
         const char *filename;
@@ -8476,7 +8476,11 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
     };
 
     if (is_proc_myself(pathname, "exe")) {
-        return safe_openat(dirfd, exec_path, flags, mode);
+        if (safe) {
+            return safe_openat(dirfd, exec_path, flags, mode);
+        } else {
+            return openat(dirfd, exec_path, flags, mode);
+        }
     }
 
     for (fake_open = fakes; fake_open->filename; fake_open++) {
@@ -8518,7 +8522,11 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
         return fd;
     }
 
-    return safe_openat(dirfd, path(pathname), flags, mode);
+    if (safe) {
+        return safe_openat(dirfd, path(pathname), flags, mode);
+    } else {
+        return openat(dirfd, path(pathname), flags, mode);
+    }
 }
 
 ssize_t do_guest_readlink(const char *pathname, char *buf, size_t bufsiz)
@@ -9027,7 +9035,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
             return -TARGET_EFAULT;
         ret = get_errno(do_guest_openat(cpu_env, AT_FDCWD, p,
                                   target_to_host_bitmask(arg2, fcntl_flags_tbl),
-                                  arg3));
+                                  arg3, true));
         fd_trans_unregister(ret);
         unlock_user(p, arg1, 0);
         return ret;
@@ -9037,7 +9045,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1,
             return -TARGET_EFAULT;
         ret = get_errno(do_guest_openat(cpu_env, arg1, p,
                                   target_to_host_bitmask(arg3, fcntl_flags_tbl),
-                                  arg4));
+                                  arg4, true));
         fd_trans_unregister(ret);
         unlock_user(p, arg2, 0);
         return ret;
-- 
2.40.1

[PATCH v3 3/8] linux-user: Emulate /proc/self/smaps

[Ilya Leoshkevich]

/proc/self/smaps is an extension of /proc/self/maps: it provides the
same lines, plus additional information about each range.

GDB uses /proc/self/smaps when available, which means that
generate-core-file tries it first before falling back to
/proc/self/maps. This, in turn, causes it to dump the host mappings,
since /proc/self/smaps is not emulated and is just passed through.

Fix by emulating /proc/self/smaps. Provide true values only for
Size, KernelPageSize, MMUPageSize and VmFlags. Leave all other values
at 0, which is a valid conservative estimate.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/syscall.c | 58 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 28a0b1f7882..c1045ea7925 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8042,7 +8042,36 @@ static int open_self_cmdline(CPUArchState *cpu_env, int fd)
     return 0;
 }
 
-static int open_self_maps(CPUArchState *cpu_env, int fd)
+static void show_smaps(int fd, unsigned long size)
+{
+    unsigned long page_size_kb = TARGET_PAGE_SIZE >> 10;
+    unsigned long size_kb = size >> 10;
+
+    dprintf(fd, "Size:                  %lu kB\n"
+                "KernelPageSize:        %lu kB\n"
+                "MMUPageSize:           %lu kB\n"
+                "Rss:                   0 kB\n"
+                "Pss:                   0 kB\n"
+                "Pss_Dirty:             0 kB\n"
+                "Shared_Clean:          0 kB\n"
+                "Shared_Dirty:          0 kB\n"
+                "Private_Clean:         0 kB\n"
+                "Private_Dirty:         0 kB\n"
+                "Referenced:            0 kB\n"
+                "Anonymous:             0 kB\n"
+                "LazyFree:              0 kB\n"
+                "AnonHugePages:         0 kB\n"
+                "ShmemPmdMapped:        0 kB\n"
+                "FilePmdMapped:         0 kB\n"
+                "Shared_Hugetlb:        0 kB\n"
+                "Private_Hugetlb:       0 kB\n"
+                "Swap:                  0 kB\n"
+                "SwapPss:               0 kB\n"
+                "Locked:                0 kB\n"
+                "THPeligible:    0\n", size_kb, page_size_kb, page_size_kb);
+}
+
+static int open_self_maps_1(CPUArchState *cpu_env, int fd, bool smaps)
 {
     CPUState *cpu = env_cpu(cpu_env);
     TaskState *ts = cpu->opaque;
@@ -8089,6 +8118,18 @@ static int open_self_maps(CPUArchState *cpu_env, int fd)
             } else {
                 dprintf(fd, "\n");
             }
+            if (smaps) {
+                show_smaps(fd, max - min);
+                dprintf(fd, "VmFlags:%s%s%s%s%s%s%s%s\n",
+                        (flags & PAGE_READ) ? " rd" : "",
+                        (flags & PAGE_WRITE_ORG) ? " wr" : "",
+                        (flags & PAGE_EXEC) ? " ex" : "",
+                        e->is_priv ? "" : " sh",
+                        (flags & PAGE_READ) ? " mr" : "",
+                        (flags & PAGE_WRITE_ORG) ? " mw" : "",
+                        (flags & PAGE_EXEC) ? " me" : "",
+                        e->is_priv ? "" : " ms");
+            }
         }
     }
 
@@ -8103,11 +8144,25 @@ static int open_self_maps(CPUArchState *cpu_env, int fd)
                     " --xp 00000000 00:00 0",
                     TARGET_VSYSCALL_PAGE, TARGET_VSYSCALL_PAGE + TARGET_PAGE_SIZE);
     dprintf(fd, "%*s%s\n", 73 - count, "",  "[vsyscall]");
+    if (smaps) {
+        show_smaps(fd, TARGET_PAGE_SIZE);
+        dprintf(fd, "VmFlags: ex\n");
+    }
 #endif
 
     return 0;
 }
 
+static int open_self_maps(CPUArchState *cpu_env, int fd)
+{
+    return open_self_maps_1(cpu_env, fd, false);
+}
+
+static int open_self_smaps(CPUArchState *cpu_env, int fd)
+{
+    return open_self_maps_1(cpu_env, fd, true);
+}
+
 static int open_self_stat(CPUArchState *cpu_env, int fd)
 {
     CPUState *cpu = env_cpu(cpu_env);
@@ -8459,6 +8514,7 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
     const struct fake_open *fake_open;
     static const struct fake_open fakes[] = {
         { "maps", open_self_maps, is_proc_myself },
+        { "smaps", open_self_smaps, is_proc_myself },
         { "stat", open_self_stat, is_proc_myself },
         { "auxv", open_self_auxv, is_proc_myself },
         { "cmdline", open_self_cmdline, is_proc_myself },
-- 
2.40.1

[PATCH v3 4/8] gdbstub: Expose gdb_get_process() and gdb_get_first_cpu_in_process()

[Ilya Leoshkevich]

These functions will be needed by user-target.c in order to retrieve
the name of the executable.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/gdbstub.c   | 16 ++++++++--------
 gdbstub/internals.h |  2 ++
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index be18568d0af..9139fec92af 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -211,7 +211,7 @@ static uint32_t gdb_get_cpu_pid(CPUState *cpu)
     return cpu->cluster_index + 1;
 }
 
-static GDBProcess *gdb_get_process(uint32_t pid)
+GDBProcess *gdb_get_process(uint32_t pid)
 {
     int i;
 
@@ -247,7 +247,7 @@ static CPUState *find_cpu(uint32_t thread_id)
     return NULL;
 }
 
-static CPUState *get_first_cpu_in_process(GDBProcess *process)
+CPUState *gdb_get_first_cpu_in_process(GDBProcess *process)
 {
     CPUState *cpu;
 
@@ -325,7 +325,7 @@ static CPUState *gdb_get_cpu(uint32_t pid, uint32_t tid)
             return NULL;
         }
 
-        return get_first_cpu_in_process(process);
+        return gdb_get_first_cpu_in_process(process);
     } else {
         /* a specific thread */
         cpu = find_cpu(tid);
@@ -354,7 +354,7 @@ static const char *get_feature_xml(const char *p, const char **newp,
     size_t len;
     int i;
     const char *name;
-    CPUState *cpu = get_first_cpu_in_process(process);
+    CPUState *cpu = gdb_get_first_cpu_in_process(process);
     CPUClass *cc = CPU_GET_CLASS(cpu);
 
     len = 0;
@@ -490,7 +490,7 @@ void gdb_register_coprocessor(CPUState *cpu,
 
 static void gdb_process_breakpoint_remove_all(GDBProcess *p)
 {
-    CPUState *cpu = get_first_cpu_in_process(p);
+    CPUState *cpu = gdb_get_first_cpu_in_process(p);
 
     while (cpu) {
         gdb_breakpoint_remove_all(cpu);
@@ -653,7 +653,7 @@ static int gdb_handle_vcont(const char *p)
                 goto out;
             }
 
-            cpu = get_first_cpu_in_process(process);
+            cpu = gdb_get_first_cpu_in_process(process);
             while (cpu) {
                 if (newstates[cpu->cpu_index] == 1) {
                     newstates[cpu->cpu_index] = cur_action;
@@ -1280,7 +1280,7 @@ static void handle_v_attach(GArray *params, void *user_ctx)
         goto cleanup;
     }
 
-    cpu = get_first_cpu_in_process(process);
+    cpu = gdb_get_first_cpu_in_process(process);
     if (!cpu) {
         goto cleanup;
     }
@@ -1403,7 +1403,7 @@ static void handle_query_curr_tid(GArray *params, void *user_ctx)
      * first thread).
      */
     process = gdb_get_cpu_process(gdbserver_state.g_cpu);
-    cpu = get_first_cpu_in_process(process);
+    cpu = gdb_get_first_cpu_in_process(process);
     g_string_assign(gdbserver_state.str_buf, "QC");
     gdb_append_thread_id(cpu, gdbserver_state.str_buf);
     gdb_put_strbuf();
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 33d21d64886..25e4d5eeaa6 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -129,6 +129,8 @@ void gdb_read_byte(uint8_t ch);
  */
 bool gdb_got_immediate_ack(void);
 /* utility helpers */
+GDBProcess *gdb_get_process(uint32_t pid);
+CPUState *gdb_get_first_cpu_in_process(GDBProcess *process);
 CPUState *gdb_first_attached_cpu(void);
 void gdb_append_thread_id(CPUState *cpu, GString *buf);
 int gdb_get_cpu_index(CPUState *cpu);
-- 
2.40.1

[PATCH v3 5/8] gdbstub: Report the actual qemu-user pid

[Ilya Leoshkevich]

Currently qemu-user reports pid 1 to GDB. Resolve the TODO and report
the actual PID. Using getpid() relies on the assumption that there is
only one GDBProcess. Add an assertion to make sure that future changes
don't break it.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/gdbstub.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 9139fec92af..c7e3ee71f2f 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -202,13 +202,16 @@ void gdb_memtox(GString *buf, const char *mem, int len)
 
 static uint32_t gdb_get_cpu_pid(CPUState *cpu)
 {
-    /* TODO: In user mode, we should use the task state PID */
+#ifdef CONFIG_USER_ONLY
+    return getpid();
+#else
     if (cpu->cluster_index == UNASSIGNED_CLUSTER_INDEX) {
         /* Return the default process' PID */
         int index = gdbserver_state.process_num - 1;
         return gdbserver_state.processes[index].pid;
     }
     return cpu->cluster_index + 1;
+#endif
 }
 
 GDBProcess *gdb_get_process(uint32_t pid)
@@ -2146,19 +2149,25 @@ void gdb_read_byte(uint8_t ch)
 void gdb_create_default_process(GDBState *s)
 {
     GDBProcess *process;
-    int max_pid = 0;
+    int pid;
 
+#ifdef CONFIG_USER_ONLY
+    assert(gdbserver_state.process_num == 0);
+    pid = getpid();
+#else
     if (gdbserver_state.process_num) {
-        max_pid = s->processes[s->process_num - 1].pid;
+        pid = s->processes[s->process_num - 1].pid;
+    } else {
+        pid = 0;
     }
+    /* We need an available PID slot for this process */
+    assert(pid < UINT32_MAX);
+    pid++;
+#endif
 
     s->processes = g_renew(GDBProcess, s->processes, ++s->process_num);
     process = &s->processes[s->process_num - 1];
-
-    /* We need an available PID slot for this process */
-    assert(max_pid < UINT32_MAX);
-
-    process->pid = max_pid + 1;
+    process->pid = pid;
     process->attached = false;
     process->target_xml[0] = '\0';
 }
-- 
2.40.1

[PATCH v3 6/8] gdbstub: Add support for info proc mappings

[Ilya Leoshkevich]

Currently the GDB's generate-core-file command doesn't work well with
qemu-user: the resulting dumps are huge [1] and at the same time
incomplete (argv and envp are missing). The reason is that GDB has no
access to proc mappings and therefore has to fall back to using
heuristics for discovering them. This is, in turn, because qemu-user
does not implement the Host I/O feature of the GDB Remote Serial
Protocol.

Implement vFile:{open,close,pread,readlink} and also
qXfer:exec-file:read+. With that, generate-core-file begins to work on
aarch64 and s390x.

[1] https://sourceware.org/pipermail/gdb-patches/2023-May/199432.html

Co-developed-by: Dominik 'Disconnect3d' Czarnota <dominik.b.czarnota@gmail.com>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 gdbstub/gdbstub.c     |  45 +++++++++++++-
 gdbstub/internals.h   |   5 ++
 gdbstub/user-target.c | 139 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 187 insertions(+), 2 deletions(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index c7e3ee71f2f..d2efefd3528 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1337,6 +1337,36 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
         .cmd = "Kill;",
         .cmd_startswith = 1
     },
+#ifdef CONFIG_USER_ONLY
+    /*
+     * Host I/O Packets. See [1] for details.
+     * [1] https://sourceware.org/gdb/onlinedocs/gdb/Host-I_002fO-Packets.html
+     */
+    {
+        .handler = gdb_handle_v_file_open,
+        .cmd = "File:open:",
+        .cmd_startswith = 1,
+        .schema = "s,L,L0"
+    },
+    {
+        .handler = gdb_handle_v_file_close,
+        .cmd = "File:close:",
+        .cmd_startswith = 1,
+        .schema = "l0"
+    },
+    {
+        .handler = gdb_handle_v_file_pread,
+        .cmd = "File:pread:",
+        .cmd_startswith = 1,
+        .schema = "l,L,L0"
+    },
+    {
+        .handler = gdb_handle_v_file_readlink,
+        .cmd = "File:readlink:",
+        .cmd_startswith = 1,
+        .schema = "s0"
+    },
+#endif
 };
 
 static void handle_v_commands(GArray *params, void *user_ctx)
@@ -1482,11 +1512,14 @@ static void handle_query_supported(GArray *params, void *user_ctx)
             ";ReverseStep+;ReverseContinue+");
     }
 
-#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
+#if defined(CONFIG_USER_ONLY)
+#if defined(CONFIG_LINUX)
     if (gdbserver_state.c_cpu->opaque) {
         g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
     }
 #endif
+    g_string_append(gdbserver_state.str_buf, ";qXfer:exec-file:read+");
+#endif
 
     if (params->len &&
         strstr(get_param(params, 0)->data, "multiprocess+")) {
@@ -1625,13 +1658,21 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
         .cmd_startswith = 1,
         .schema = "s:l,l0"
     },
-#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
+#if defined(CONFIG_USER_ONLY)
+#if defined(CONFIG_LINUX)
     {
         .handler = gdb_handle_query_xfer_auxv,
         .cmd = "Xfer:auxv:read::",
         .cmd_startswith = 1,
         .schema = "l,l0"
     },
+#endif
+    {
+        .handler = gdb_handle_query_xfer_exec_file,
+        .cmd = "Xfer:exec-file:read:",
+        .cmd_startswith = 1,
+        .schema = "l:l,l0"
+    },
 #endif
     {
         .handler = gdb_handle_query_attached,
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 25e4d5eeaa6..f2b46cce412 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -189,6 +189,11 @@ typedef union GdbCmdVariant {
 void gdb_handle_query_rcmd(GArray *params, void *user_ctx); /* softmmu */
 void gdb_handle_query_offsets(GArray *params, void *user_ctx); /* user */
 void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx); /*user */
+void gdb_handle_v_file_open(GArray *params, void *user_ctx); /* user */
+void gdb_handle_v_file_close(GArray *params, void *user_ctx); /* user */
+void gdb_handle_v_file_pread(GArray *params, void *user_ctx); /* user */
+void gdb_handle_v_file_readlink(GArray *params, void *user_ctx); /* user */
+void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx); /* user */
 
 void gdb_handle_query_attached(GArray *params, void *user_ctx); /* both */
 
diff --git a/gdbstub/user-target.c b/gdbstub/user-target.c
index fa0e59ec9a5..aa64a8b9440 100644
--- a/gdbstub/user-target.c
+++ b/gdbstub/user-target.c
@@ -11,6 +11,10 @@
 #include "exec/gdbstub.h"
 #include "qemu.h"
 #include "internals.h"
+#ifdef CONFIG_LINUX
+#include "linux-user/loader.h"
+#include "linux-user/qemu.h"
+#endif
 
 /*
  * Map target signal numbers to GDB protocol signal numbers and vice
@@ -281,3 +285,138 @@ void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx)
                       gdbserver_state.str_buf->len, true);
 }
 #endif
+
+static const char *get_filename_param(GArray *params, int i)
+{
+    const char *hex_filename = get_param(params, i)->data;
+    gdb_hextomem(gdbserver_state.mem_buf, hex_filename,
+                 strlen(hex_filename) / 2);
+    g_byte_array_append(gdbserver_state.mem_buf, (const guint8 *)"", 1);
+    return (const char *)gdbserver_state.mem_buf->data;
+}
+
+static void hostio_reply_with_data(const void *buf, size_t n)
+{
+    g_string_printf(gdbserver_state.str_buf, "F%lx;", n);
+    gdb_memtox(gdbserver_state.str_buf, buf, n);
+    gdb_put_packet_binary(gdbserver_state.str_buf->str,
+                          gdbserver_state.str_buf->len, true);
+}
+
+void gdb_handle_v_file_open(GArray *params, void *user_ctx)
+{
+    const char *filename = get_filename_param(params, 0);
+    uint64_t flags = get_param(params, 1)->val_ull;
+    uint64_t mode = get_param(params, 2)->val_ull;
+
+#ifdef CONFIG_LINUX
+    int fd = do_guest_openat(gdbserver_state.g_cpu->env_ptr, 0, filename,
+                             flags, mode, false);
+#else
+    int fd = open(filename, flags, mode);
+#endif
+    if (fd < 0) {
+        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
+    } else {
+        g_string_printf(gdbserver_state.str_buf, "F%d", fd);
+    }
+    gdb_put_strbuf();
+}
+
+void gdb_handle_v_file_close(GArray *params, void *user_ctx)
+{
+    int fd = get_param(params, 0)->val_ul;
+
+    if (close(fd) == -1) {
+        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
+        gdb_put_strbuf();
+        return;
+    }
+
+    gdb_put_packet("F00");
+}
+
+#define BUFSIZ 8192
+
+void gdb_handle_v_file_pread(GArray *params, void *user_ctx)
+{
+    int fd = get_param(params, 0)->val_ul;
+    size_t count = get_param(params, 1)->val_ull;
+    off_t offset = get_param(params, 2)->val_ull;
+
+    size_t bufsiz = MIN(count, BUFSIZ);
+    g_autofree char *buf = g_try_malloc(bufsiz);
+    if (buf == NULL) {
+        gdb_put_packet("E12");
+        return;
+    }
+
+    ssize_t n = pread(fd, buf, bufsiz, offset);
+    if (n < 0) {
+        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
+        gdb_put_strbuf();
+        return;
+    }
+    hostio_reply_with_data(buf, n);
+}
+
+void gdb_handle_v_file_readlink(GArray *params, void *user_ctx)
+{
+    const char *filename = get_filename_param(params, 0);
+
+    g_autofree char *buf = g_try_malloc(BUFSIZ);
+    if (buf == NULL) {
+        gdb_put_packet("E12");
+        return;
+    }
+
+#ifdef CONFIG_LINUX
+    ssize_t n = do_guest_readlink(filename, buf, BUFSIZ);
+#else
+    ssize_t n = readlink(filename, buf, BUFSIZ);
+#endif
+    if (n < 0) {
+        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
+        gdb_put_strbuf();
+        return;
+    }
+    hostio_reply_with_data(buf, n);
+}
+
+void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx)
+{
+    uint32_t pid = get_param(params, 0)->val_ul;
+    uint32_t offset = get_param(params, 1)->val_ul;
+    uint32_t length = get_param(params, 2)->val_ul;
+
+    GDBProcess *process = gdb_get_process(pid);
+    if (!process) {
+        gdb_put_packet("E00");
+        return;
+    }
+
+    CPUState *cpu = gdb_get_first_cpu_in_process(process);
+    if (!cpu) {
+        gdb_put_packet("E00");
+        return;
+    }
+
+    TaskState *ts = cpu->opaque;
+    if (!ts || !ts->bprm || !ts->bprm->filename) {
+        gdb_put_packet("E00");
+        return;
+    }
+
+    size_t total_length = strlen(ts->bprm->filename);
+    if (offset > total_length) {
+        gdb_put_packet("E00");
+        return;
+    }
+    if (offset + length > total_length) {
+        length = total_length - offset;
+    }
+
+    g_string_printf(gdbserver_state.str_buf, "l%.*s", length,
+                    ts->bprm->filename + offset);
+    gdb_put_strbuf();
+}
-- 
2.40.1

[PATCH v3 7/8] docs: Document security implications of debugging

[Ilya Leoshkevich]

Now that the GDB stub explicitly implements reading host files (note
that it was already possible by changing the emulated code to open and
read those files), concerns may arise that it undermines security.

Document the status quo, which is that the users are already
responsible for securing the GDB connection themselves.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 docs/system/gdb.rst | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/docs/system/gdb.rst b/docs/system/gdb.rst
index 7d3718deefb..9906991b841 100644
--- a/docs/system/gdb.rst
+++ b/docs/system/gdb.rst
@@ -214,3 +214,18 @@ The memory mode can be checked by sending the following command:
 
 ``maintenance packet Qqemu.PhyMemMode:0``
     This will change it back to normal memory mode.
+
+Security considerations
+=======================
+
+Connecting to the GDB socket allows running arbitrary code inside the guest;
+in case of the TCG emulation, which is not considered a security boundary, this
+also means running arbitrary code on the host. Additionally, when debugging
+qemu-user, it allows directly downloading any file readable by QEMU from the
+host.
+
+The GDB socket is not protected by authentication, authorization or encryption.
+It is therefore a responsibility of the user to make sure that only authorized
+clients can connect to it, e.g., by using a unix socket with proper
+permissions, or by opening a TCP socket only on interfaces that are not
+reachable by potential attackers.
-- 
2.40.1

[PATCH v3 8/8] tests/tcg: Add a test for info proc mappings

[Ilya Leoshkevich]

Add a small test to prevent regressions.
Since there are issues with how GDB interprets QEMU's target.xml,
enable the test only on aarch64 and s390x for now.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/aarch64/Makefile.target             |  3 +-
 tests/tcg/multiarch/Makefile.target           |  7 +++
 .../multiarch/gdbstub/test-proc-mappings.py   | 55 +++++++++++++++++++
 tests/tcg/s390x/Makefile.target               |  2 +-
 4 files changed, 65 insertions(+), 2 deletions(-)
 create mode 100644 tests/tcg/multiarch/gdbstub/test-proc-mappings.py

diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 03157954871..38402b0ba1f 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -97,7 +97,8 @@ run-gdbstub-sve-ioctls: sve-ioctls
 		--bin $< --test $(AARCH64_SRC)/gdbstub/test-sve-ioctl.py, \
 	basic gdbstub SVE ZLEN support)
 
-EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
+EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls \
+              run-gdbstub-proc-mappings
 endif
 endif
 
diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
index 373db696481..cbc0b75787a 100644
--- a/tests/tcg/multiarch/Makefile.target
+++ b/tests/tcg/multiarch/Makefile.target
@@ -81,6 +81,13 @@ run-gdbstub-qxfer-auxv-read: sha1
 		--bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \
 	basic gdbstub qXfer:auxv:read support)
 
+run-gdbstub-proc-mappings: sha1
+	$(call run-test, $@, $(GDB_SCRIPT) \
+		--gdb $(HAVE_GDB_BIN) \
+		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
+		--bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-mappings.py, \
+	proc mappings support)
+
 run-gdbstub-thread-breakpoint: testthread
 	$(call run-test, $@, $(GDB_SCRIPT) \
 		--gdb $(HAVE_GDB_BIN) \
diff --git a/tests/tcg/multiarch/gdbstub/test-proc-mappings.py b/tests/tcg/multiarch/gdbstub/test-proc-mappings.py
new file mode 100644
index 00000000000..657e36a2fc7
--- /dev/null
+++ b/tests/tcg/multiarch/gdbstub/test-proc-mappings.py
@@ -0,0 +1,55 @@
+"""Test that gdbstub has access to proc mappings.
+
+This runs as a sourced script (via -x, via run-test.py)."""
+from __future__ import print_function
+import gdb
+import sys
+
+
+n_failures = 0
+
+
+def report(cond, msg):
+    """Report success/fail of a test"""
+    if cond:
+        print("PASS: {}".format(msg))
+    else:
+        print("FAIL: {}".format(msg))
+        global n_failures
+        n_failures += 1
+
+
+def run_test():
+    """Run through the tests one by one"""
+    mappings = gdb.execute("info proc mappings", False, True)
+    report(isinstance(mappings, str), "Fetched the mappings from the inferior")
+    report("/sha1" in mappings, "Found the test binary name in the mappings")
+
+
+def main():
+    """Prepare the environment and run through the tests"""
+    try:
+        inferior = gdb.selected_inferior()
+        print("ATTACHED: {}".format(inferior.architecture().name()))
+    except (gdb.error, AttributeError):
+        print("SKIPPING (not connected)")
+        exit(0)
+
+    if gdb.parse_and_eval('$pc') == 0:
+        print("SKIP: PC not set")
+        exit(0)
+
+    try:
+        # These are not very useful in scripts
+        gdb.execute("set pagination off")
+        gdb.execute("set confirm off")
+
+        # Run the actual tests
+        run_test()
+    except gdb.error:
+        report(False, "GDB Exception: {}".format(sys.exc_info()[0]))
+    print("All tests complete: %d failures" % n_failures)
+    exit(n_failures)
+
+
+main()
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 4899503e1db..d33960caa0a 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -78,7 +78,7 @@ run-gdbstub-signals-s390x: signals-s390x
 		--bin $< --test $(S390X_SRC)/gdbstub/test-signals-s390x.py, \
 	mixing signals and debugging)
 
-EXTRA_RUNS += run-gdbstub-signals-s390x
+EXTRA_RUNS += run-gdbstub-signals-s390x run-gdbstub-proc-mappings
 endif
 
 # MVX versions of sha512
-- 
2.40.1

Re: [PATCH v3 1/8] linux-user: Expose do_guest_openat() and do_guest_readlink()

[Richard Henderson]

On 6/6/23 06:27, Ilya Leoshkevich wrote:
> These functions will be required by the GDB stub in order to provide
> the guest view of /proc to GDB.
> 
> Reviewed-by: Alex Bennée<alex.bennee@linaro.org>
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   linux-user/qemu.h    |  3 +++
>   linux-user/syscall.c | 54 ++++++++++++++++++++++++++++----------------
>   2 files changed, 38 insertions(+), 19 deletions(-)

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

r~

Re: [PATCH v3 3/8] linux-user: Emulate /proc/self/smaps

[Richard Henderson]

On 6/6/23 06:27, Ilya Leoshkevich wrote:
> /proc/self/smaps is an extension of /proc/self/maps: it provides the
> same lines, plus additional information about each range.
> 
> GDB uses /proc/self/smaps when available, which means that
> generate-core-file tries it first before falling back to
> /proc/self/maps. This, in turn, causes it to dump the host mappings,
> since /proc/self/smaps is not emulated and is just passed through.
> 
> Fix by emulating /proc/self/smaps. Provide true values only for
> Size, KernelPageSize, MMUPageSize and VmFlags. Leave all other values
> at 0, which is a valid conservative estimate.
> 
> Signed-off-by: Ilya Leoshkevich<iii@linux.ibm.com>
> ---
>   linux-user/syscall.c | 58 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 57 insertions(+), 1 deletion(-)

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

> +            if (smaps) {
> +                show_smaps(fd, max - min);
> +                dprintf(fd, "VmFlags:%s%s%s%s%s%s%s%s\n",
> +                        (flags & PAGE_READ) ? " rd" : "",
> +                        (flags & PAGE_WRITE_ORG) ? " wr" : "",
> +                        (flags & PAGE_EXEC) ? " ex" : "",
> +                        e->is_priv ? "" : " sh",
> +                        (flags & PAGE_READ) ? " mr" : "",
> +                        (flags & PAGE_WRITE_ORG) ? " mw" : "",
> +                        (flags & PAGE_EXEC) ? " me" : "",
> +                        e->is_priv ? "" : " ms");
> +            }

There are a couple of AArch64 specific bits here, which we emulate.
But this is good enough for now.


r~

Re: [PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat()

[Richard Henderson]

On 6/6/23 06:27, Ilya Leoshkevich wrote:
> @@ -8518,7 +8522,11 @@ int do_guest_openat(CPUArchState *cpu_env, int dirfd, const char *pathname,
>           return fd;
>       }
>   
> -    return safe_openat(dirfd, path(pathname), flags, mode);
> +    if (safe) {
> +        return safe_openat(dirfd, path(pathname), flags, mode);
> +    } else {
> +        return openat(dirfd, path(pathname), flags, mode);
> +    }
>   }

I'm not keen on this, as it seems like the wrong abstraction.  But I can't immediately 
think of how it could be better structured.

The only concrete objection I have is the change of API, which could be fixed with return 
get_errno(openat(...)).

With that,

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


r~

Re: [PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat()

[Ilya Leoshkevich]

On Tue, 2023-06-06 at 11:24 -0700, Richard Henderson wrote:
> On 6/6/23 06:27, Ilya Leoshkevich wrote:
> > @@ -8518,7 +8522,11 @@ int do_guest_openat(CPUArchState *cpu_env,
> > int dirfd, const char *pathname,
> >           return fd;
> >       }
> >   
> > -    return safe_openat(dirfd, path(pathname), flags, mode);
> > +    if (safe) {
> > +        return safe_openat(dirfd, path(pathname), flags, mode);
> > +    } else {
> > +        return openat(dirfd, path(pathname), flags, mode);
> > +    }
> >   }
> 
> I'm not keen on this, as it seems like the wrong abstraction.  But I
> can't immediately 
> think of how it could be better structured.

I also thought about temporarily clearing signal_pending in gdbstub,
but could not convince myself that this were safe.

> The only concrete objection I have is the change of API, which could
> be fixed with return 
> get_errno(openat(...)).

I believe both openat() and safe_openat() return -1 on error and set
errno, or am I missing something?

> 
> With that,
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> 
> r~

Re: [PATCH v3 2/8] linux-user: Add "safe" parameter to do_guest_openat()

[Richard Henderson]

On 6/6/23 12:29, Ilya Leoshkevich wrote:
>> The only concrete objection I have is the change of API, which could
>> be fixed with return
>> get_errno(openat(...)).
> 
> I believe both openat() and safe_openat() return -1 on error and set
> errno, or am I missing something?
Oops, no, bad memory on my part -- I thought safe_foo returned -errno.


r~

Re: [PATCH v3 6/8] gdbstub: Add support for info proc mappings

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> Currently the GDB's generate-core-file command doesn't work well with
> qemu-user: the resulting dumps are huge [1] and at the same time
> incomplete (argv and envp are missing). The reason is that GDB has no
> access to proc mappings and therefore has to fall back to using
> heuristics for discovering them. This is, in turn, because qemu-user
> does not implement the Host I/O feature of the GDB Remote Serial
> Protocol.
>
> Implement vFile:{open,close,pread,readlink} and also
> qXfer:exec-file:read+. With that, generate-core-file begins to work on
> aarch64 and s390x.
>
> [1] https://sourceware.org/pipermail/gdb-patches/2023-May/199432.html
>
> Co-developed-by: Dominik 'Disconnect3d' Czarnota <dominik.b.czarnota@gmail.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  gdbstub/gdbstub.c     |  45 +++++++++++++-
>  gdbstub/internals.h   |   5 ++
>  gdbstub/user-target.c | 139 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 187 insertions(+), 2 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index c7e3ee71f2f..d2efefd3528 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -1337,6 +1337,36 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
>          .cmd = "Kill;",
>          .cmd_startswith = 1
>      },
> +#ifdef CONFIG_USER_ONLY
> +    /*
> +     * Host I/O Packets. See [1] for details.
> +     * [1] https://sourceware.org/gdb/onlinedocs/gdb/Host-I_002fO-Packets.html
> +     */
> +    {
> +        .handler = gdb_handle_v_file_open,
> +        .cmd = "File:open:",
> +        .cmd_startswith = 1,
> +        .schema = "s,L,L0"
> +    },
> +    {
> +        .handler = gdb_handle_v_file_close,
> +        .cmd = "File:close:",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
> +    {
> +        .handler = gdb_handle_v_file_pread,
> +        .cmd = "File:pread:",
> +        .cmd_startswith = 1,
> +        .schema = "l,L,L0"
> +    },
> +    {
> +        .handler = gdb_handle_v_file_readlink,
> +        .cmd = "File:readlink:",
> +        .cmd_startswith = 1,
> +        .schema = "s0"
> +    },
> +#endif
>  };
>  
>  static void handle_v_commands(GArray *params, void *user_ctx)
> @@ -1482,11 +1512,14 @@ static void handle_query_supported(GArray *params, void *user_ctx)
>              ";ReverseStep+;ReverseContinue+");
>      }
>  
> -#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
> +#if defined(CONFIG_USER_ONLY)
> +#if defined(CONFIG_LINUX)
>      if (gdbserver_state.c_cpu->opaque) {
>          g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
>      }
>  #endif
> +    g_string_append(gdbserver_state.str_buf, ";qXfer:exec-file:read+");
> +#endif
>  
>      if (params->len &&
>          strstr(get_param(params, 0)->data, "multiprocess+")) {
> @@ -1625,13 +1658,21 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
>          .cmd_startswith = 1,
>          .schema = "s:l,l0"
>      },
> -#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
> +#if defined(CONFIG_USER_ONLY)
> +#if defined(CONFIG_LINUX)
>      {
>          .handler = gdb_handle_query_xfer_auxv,
>          .cmd = "Xfer:auxv:read::",
>          .cmd_startswith = 1,
>          .schema = "l,l0"
>      },
> +#endif
> +    {
> +        .handler = gdb_handle_query_xfer_exec_file,
> +        .cmd = "Xfer:exec-file:read:",
> +        .cmd_startswith = 1,
> +        .schema = "l:l,l0"
> +    },
>  #endif
>      {
>          .handler = gdb_handle_query_attached,
> diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> index 25e4d5eeaa6..f2b46cce412 100644
> --- a/gdbstub/internals.h
> +++ b/gdbstub/internals.h
> @@ -189,6 +189,11 @@ typedef union GdbCmdVariant {
>  void gdb_handle_query_rcmd(GArray *params, void *user_ctx); /* softmmu */
>  void gdb_handle_query_offsets(GArray *params, void *user_ctx); /* user */
>  void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx); /*user */
> +void gdb_handle_v_file_open(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_v_file_close(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_v_file_pread(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_v_file_readlink(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx); /* user */
>  
>  void gdb_handle_query_attached(GArray *params, void *user_ctx); /* both */
>  
> diff --git a/gdbstub/user-target.c b/gdbstub/user-target.c
> index fa0e59ec9a5..aa64a8b9440 100644
> --- a/gdbstub/user-target.c
> +++ b/gdbstub/user-target.c
> @@ -11,6 +11,10 @@
>  #include "exec/gdbstub.h"
>  #include "qemu.h"
>  #include "internals.h"
> +#ifdef CONFIG_LINUX
> +#include "linux-user/loader.h"
> +#include "linux-user/qemu.h"
> +#endif
>  
>  /*
>   * Map target signal numbers to GDB protocol signal numbers and vice
> @@ -281,3 +285,138 @@ void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx)
>                        gdbserver_state.str_buf->len, true);
>  }
>  #endif
> +
> +static const char *get_filename_param(GArray *params, int i)
> +{
> +    const char *hex_filename = get_param(params, i)->data;
> +    gdb_hextomem(gdbserver_state.mem_buf, hex_filename,
> +                 strlen(hex_filename) / 2);
> +    g_byte_array_append(gdbserver_state.mem_buf, (const guint8 *)"", 1);
> +    return (const char *)gdbserver_state.mem_buf->data;
> +}
> +
> +static void hostio_reply_with_data(const void *buf, size_t n)
> +{
> +    g_string_printf(gdbserver_state.str_buf, "F%lx;", n);
> +    gdb_memtox(gdbserver_state.str_buf, buf, n);
> +    gdb_put_packet_binary(gdbserver_state.str_buf->str,
> +                          gdbserver_state.str_buf->len, true);
> +}

This fails on 64/32 builds:

  ../gdbstub/user-target.c: In function ‘hostio_reply_with_data’:
  ../gdbstub/user-target.c:300:50: error: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘size_t’ {aka ‘unsigned int’} [-Werror=format=]
    300 |     g_string_printf(gdbserver_state.str_buf, "F%lx;", n);
        |                                                ~~^    ~
        |                                                  |    |
        |                                                  |    size_t {aka unsigned int}
        |                                                  long unsigned int
        |                                                %x
  cc1: all warnings being treated as errors

I think "%zx" is the portable format string you want.


> +void gdb_handle_v_file_open(GArray *params, void *user_ctx)
> +{
> +    const char *filename = get_filename_param(params, 0);
> +    uint64_t flags = get_param(params, 1)->val_ull;
> +    uint64_t mode = get_param(params, 2)->val_ull;
> +
> +#ifdef CONFIG_LINUX
> +    int fd = do_guest_openat(gdbserver_state.g_cpu->env_ptr, 0, filename,
> +                             flags, mode, false);
> +#else
> +    int fd = open(filename, flags, mode);
> +#endif
> +    if (fd < 0) {
> +        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> +    } else {
> +        g_string_printf(gdbserver_state.str_buf, "F%d", fd);
> +    }
> +    gdb_put_strbuf();
> +}
> +
> +void gdb_handle_v_file_close(GArray *params, void *user_ctx)
> +{
> +    int fd = get_param(params, 0)->val_ul;
> +
> +    if (close(fd) == -1) {
> +        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> +        gdb_put_strbuf();
> +        return;
> +    }
> +
> +    gdb_put_packet("F00");
> +}
> +
> +#define BUFSIZ 8192
> +
> +void gdb_handle_v_file_pread(GArray *params, void *user_ctx)
> +{
> +    int fd = get_param(params, 0)->val_ul;
> +    size_t count = get_param(params, 1)->val_ull;
> +    off_t offset = get_param(params, 2)->val_ull;
> +
> +    size_t bufsiz = MIN(count, BUFSIZ);
> +    g_autofree char *buf = g_try_malloc(bufsiz);
> +    if (buf == NULL) {
> +        gdb_put_packet("E12");
> +        return;
> +    }
> +
> +    ssize_t n = pread(fd, buf, bufsiz, offset);
> +    if (n < 0) {
> +        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> +        gdb_put_strbuf();
> +        return;
> +    }
> +    hostio_reply_with_data(buf, n);
> +}
> +
> +void gdb_handle_v_file_readlink(GArray *params, void *user_ctx)
> +{
> +    const char *filename = get_filename_param(params, 0);
> +
> +    g_autofree char *buf = g_try_malloc(BUFSIZ);
> +    if (buf == NULL) {
> +        gdb_put_packet("E12");
> +        return;
> +    }
> +
> +#ifdef CONFIG_LINUX
> +    ssize_t n = do_guest_readlink(filename, buf, BUFSIZ);
> +#else
> +    ssize_t n = readlink(filename, buf, BUFSIZ);
> +#endif
> +    if (n < 0) {
> +        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> +        gdb_put_strbuf();
> +        return;
> +    }
> +    hostio_reply_with_data(buf, n);
> +}
> +
> +void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx)
> +{
> +    uint32_t pid = get_param(params, 0)->val_ul;
> +    uint32_t offset = get_param(params, 1)->val_ul;
> +    uint32_t length = get_param(params, 2)->val_ul;
> +
> +    GDBProcess *process = gdb_get_process(pid);
> +    if (!process) {
> +        gdb_put_packet("E00");
> +        return;
> +    }
> +
> +    CPUState *cpu = gdb_get_first_cpu_in_process(process);
> +    if (!cpu) {
> +        gdb_put_packet("E00");
> +        return;
> +    }
> +
> +    TaskState *ts = cpu->opaque;
> +    if (!ts || !ts->bprm || !ts->bprm->filename) {
> +        gdb_put_packet("E00");
> +        return;
> +    }
> +
> +    size_t total_length = strlen(ts->bprm->filename);
> +    if (offset > total_length) {
> +        gdb_put_packet("E00");
> +        return;
> +    }
> +    if (offset + length > total_length) {
> +        length = total_length - offset;
> +    }
> +
> +    g_string_printf(gdbserver_state.str_buf, "l%.*s", length,
> +                    ts->bprm->filename + offset);
> +    gdb_put_strbuf();
> +}


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 8/8] tests/tcg: Add a test for info proc mappings

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> Add a small test to prevent regressions.
> Since there are issues with how GDB interprets QEMU's target.xml,
> enable the test only on aarch64 and s390x for now.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  tests/tcg/aarch64/Makefile.target             |  3 +-
>  tests/tcg/multiarch/Makefile.target           |  7 +++
>  .../multiarch/gdbstub/test-proc-mappings.py   | 55 +++++++++++++++++++
>  tests/tcg/s390x/Makefile.target               |  2 +-
>  4 files changed, 65 insertions(+), 2 deletions(-)
>  create mode 100644 tests/tcg/multiarch/gdbstub/test-proc-mappings.py
>
> diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
> index 03157954871..38402b0ba1f 100644
> --- a/tests/tcg/aarch64/Makefile.target
> +++ b/tests/tcg/aarch64/Makefile.target
> @@ -97,7 +97,8 @@ run-gdbstub-sve-ioctls: sve-ioctls
>  		--bin $< --test $(AARCH64_SRC)/gdbstub/test-sve-ioctl.py, \
>  	basic gdbstub SVE ZLEN support)
>  
> -EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
> +EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls \
> +              run-gdbstub-proc-mappings
>  endif
>  endif
>  
> diff --git a/tests/tcg/multiarch/Makefile.target b/tests/tcg/multiarch/Makefile.target
> index 373db696481..cbc0b75787a 100644
> --- a/tests/tcg/multiarch/Makefile.target
> +++ b/tests/tcg/multiarch/Makefile.target
> @@ -81,6 +81,13 @@ run-gdbstub-qxfer-auxv-read: sha1
>  		--bin $< --test $(MULTIARCH_SRC)/gdbstub/test-qxfer-auxv-read.py, \
>  	basic gdbstub qXfer:auxv:read support)
>  
> +run-gdbstub-proc-mappings: sha1
> +	$(call run-test, $@, $(GDB_SCRIPT) \
> +		--gdb $(HAVE_GDB_BIN) \
> +		--qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
> +		--bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-mappings.py, \
> +	proc mappings support)
> +

I wondered if it makes more sense to keep the extra test configuration
logic in multiarch:

  run-gdbstub-proc-mappings: sha1
          $(call run-test, $@, $(GDB_SCRIPT) \
                  --gdb $(HAVE_GDB_BIN) \
                  --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
                  --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-mappings.py, \
          proc mappings support)

  # only enable for s390x and aarch64 for now
  ifneq (,$(findstring aarch64,$(TARGET_NAME)))
  EXTRA_RUNS += run-gdbstub-proc-mappings
  else ifneq (,$(findstring s390x,$(TARGET_NAME)))
  EXTRA_RUNS += run-gdbstub-proc-mappings
  endif

but it still ends up pretty ugly. Is the gdb handling fixed for other
arches in other versions. Maybe we could probe gdb for support and wrap
the whole stanza in something like:

  ifeq ($(HOST_GDB_SUPPORTS_PROC_MAPPING),y)
  ...
  endif

?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v3 8/8] tests/tcg: Add a test for info proc mappings

[Ilya Leoshkevich]

On Wed, 2023-06-21 at 11:21 +0100, Alex Bennée wrote:
> 
> Ilya Leoshkevich <iii@linux.ibm.com> writes:
> 
> > Add a small test to prevent regressions.
> > Since there are issues with how GDB interprets QEMU's target.xml,
> > enable the test only on aarch64 and s390x for now.
> > 
> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> > ---
> >  tests/tcg/aarch64/Makefile.target             |  3 +-
> >  tests/tcg/multiarch/Makefile.target           |  7 +++
> >  .../multiarch/gdbstub/test-proc-mappings.py   | 55
> > +++++++++++++++++++
> >  tests/tcg/s390x/Makefile.target               |  2 +-
> >  4 files changed, 65 insertions(+), 2 deletions(-)
> >  create mode 100644 tests/tcg/multiarch/gdbstub/test-proc-
> > mappings.py
> > 
> > diff --git a/tests/tcg/aarch64/Makefile.target
> > b/tests/tcg/aarch64/Makefile.target
> > index 03157954871..38402b0ba1f 100644
> > --- a/tests/tcg/aarch64/Makefile.target
> > +++ b/tests/tcg/aarch64/Makefile.target
> > @@ -97,7 +97,8 @@ run-gdbstub-sve-ioctls: sve-ioctls
> >                 --bin $< --test $(AARCH64_SRC)/gdbstub/test-sve-
> > ioctl.py, \
> >         basic gdbstub SVE ZLEN support)
> >  
> > -EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
> > +EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls \
> > +              run-gdbstub-proc-mappings
> >  endif
> >  endif
> >  
> > diff --git a/tests/tcg/multiarch/Makefile.target
> > b/tests/tcg/multiarch/Makefile.target
> > index 373db696481..cbc0b75787a 100644
> > --- a/tests/tcg/multiarch/Makefile.target
> > +++ b/tests/tcg/multiarch/Makefile.target
> > @@ -81,6 +81,13 @@ run-gdbstub-qxfer-auxv-read: sha1
> >                 --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-
> > qxfer-auxv-read.py, \
> >         basic gdbstub qXfer:auxv:read support)
> >  
> > +run-gdbstub-proc-mappings: sha1
> > +       $(call run-test, $@, $(GDB_SCRIPT) \
> > +               --gdb $(HAVE_GDB_BIN) \
> > +               --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
> > +               --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-
> > mappings.py, \
> > +       proc mappings support)
> > +
> 
> I wondered if it makes more sense to keep the extra test
> configuration
> logic in multiarch:
> 
>   run-gdbstub-proc-mappings: sha1
>           $(call run-test, $@, $(GDB_SCRIPT) \
>                   --gdb $(HAVE_GDB_BIN) \
>                   --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
>                   --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-
> mappings.py, \
>           proc mappings support)
> 
>   # only enable for s390x and aarch64 for now
>   ifneq (,$(findstring aarch64,$(TARGET_NAME)))
>   EXTRA_RUNS += run-gdbstub-proc-mappings
>   else ifneq (,$(findstring s390x,$(TARGET_NAME)))
>   EXTRA_RUNS += run-gdbstub-proc-mappings
>   endif
> 
> but it still ends up pretty ugly. Is the gdb handling fixed for other
> arches in other versions. Maybe we could probe gdb for support and
> wrap
> the whole stanza in something like:
> 
>   ifeq ($(HOST_GDB_SUPPORTS_PROC_MAPPING),y)
>   ...
>   endif
> 
> ?

I think I better add the check to the test itself, because otherwise we
have to probe GDB against QEMU binary we just built, which sounds
unnecessarily complicated.

The error message on all arches without this series is:

    warning: unable to open /proc file '/proc/1/maps'

The error message on x86_64 (expected) with this series is:

   Not supported on this target.

So we can simply exit(0) from the test if we see this.

Re: [PATCH v3 8/8] tests/tcg: Add a test for info proc mappings

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> On Wed, 2023-06-21 at 11:21 +0100, Alex Bennée wrote:
>> 
>> Ilya Leoshkevich <iii@linux.ibm.com> writes:
>> 
>> > Add a small test to prevent regressions.
>> > Since there are issues with how GDB interprets QEMU's target.xml,
>> > enable the test only on aarch64 and s390x for now.
>> > 
>> > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
>> > ---
>> >  tests/tcg/aarch64/Makefile.target             |  3 +-
>> >  tests/tcg/multiarch/Makefile.target           |  7 +++
>> >  .../multiarch/gdbstub/test-proc-mappings.py   | 55
>> > +++++++++++++++++++
>> >  tests/tcg/s390x/Makefile.target               |  2 +-
>> >  4 files changed, 65 insertions(+), 2 deletions(-)
>> >  create mode 100644 tests/tcg/multiarch/gdbstub/test-proc-
>> > mappings.py
>> > 
>> > diff --git a/tests/tcg/aarch64/Makefile.target
>> > b/tests/tcg/aarch64/Makefile.target
>> > index 03157954871..38402b0ba1f 100644
>> > --- a/tests/tcg/aarch64/Makefile.target
>> > +++ b/tests/tcg/aarch64/Makefile.target
>> > @@ -97,7 +97,8 @@ run-gdbstub-sve-ioctls: sve-ioctls
>> >                 --bin $< --test $(AARCH64_SRC)/gdbstub/test-sve-
>> > ioctl.py, \
>> >         basic gdbstub SVE ZLEN support)
>> >  
>> > -EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls
>> > +EXTRA_RUNS += run-gdbstub-sysregs run-gdbstub-sve-ioctls \
>> > +              run-gdbstub-proc-mappings
>> >  endif
>> >  endif
>> >  
>> > diff --git a/tests/tcg/multiarch/Makefile.target
>> > b/tests/tcg/multiarch/Makefile.target
>> > index 373db696481..cbc0b75787a 100644
>> > --- a/tests/tcg/multiarch/Makefile.target
>> > +++ b/tests/tcg/multiarch/Makefile.target
>> > @@ -81,6 +81,13 @@ run-gdbstub-qxfer-auxv-read: sha1
>> >                 --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-
>> > qxfer-auxv-read.py, \
>> >         basic gdbstub qXfer:auxv:read support)
>> >  
>> > +run-gdbstub-proc-mappings: sha1
>> > +       $(call run-test, $@, $(GDB_SCRIPT) \
>> > +               --gdb $(HAVE_GDB_BIN) \
>> > +               --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
>> > +               --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-
>> > mappings.py, \
>> > +       proc mappings support)
>> > +
>> 
>> I wondered if it makes more sense to keep the extra test
>> configuration
>> logic in multiarch:
>> 
>>   run-gdbstub-proc-mappings: sha1
>>           $(call run-test, $@, $(GDB_SCRIPT) \
>>                   --gdb $(HAVE_GDB_BIN) \
>>                   --qemu $(QEMU) --qargs "$(QEMU_OPTS)" \
>>                   --bin $< --test $(MULTIARCH_SRC)/gdbstub/test-proc-
>> mappings.py, \
>>           proc mappings support)
>> 
>>   # only enable for s390x and aarch64 for now
>>   ifneq (,$(findstring aarch64,$(TARGET_NAME)))
>>   EXTRA_RUNS += run-gdbstub-proc-mappings
>>   else ifneq (,$(findstring s390x,$(TARGET_NAME)))
>>   EXTRA_RUNS += run-gdbstub-proc-mappings
>>   endif
>> 
>> but it still ends up pretty ugly. Is the gdb handling fixed for other
>> arches in other versions. Maybe we could probe gdb for support and
>> wrap
>> the whole stanza in something like:
>> 
>>   ifeq ($(HOST_GDB_SUPPORTS_PROC_MAPPING),y)
>>   ...
>>   endif
>> 
>> ?
>
> I think I better add the check to the test itself, because otherwise we
> have to probe GDB against QEMU binary we just built, which sounds
> unnecessarily complicated.
>
> The error message on all arches without this series is:
>
>     warning: unable to open /proc file '/proc/1/maps'
>
> The error message on x86_64 (expected) with this series is:
>
>    Not supported on this target.
>
> So we can simply exit(0) from the test if we see this.

That seems a simpler solution, lets do that.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro