gdbstub.c: add support for info proc mappings

This commit adds support for `info proc mappings` and a few other commands into
the QEMU user-mode emulation gdbstub.

For that, support for the following GDB remote protocol commands has been added:
* vFile:setfs: pid
* vFile:open: filename, flags, mode
* vFile:pread: fd, count, offset
* vFile:close: fd
* qXfer:exec-file:read:annex:offset,length

Additionally, a `REMOTE_DEBUG_PRINT` macro has been added for printing remote debug logs.
To enable it, set the `ENABLE_REMOTE_DEBUG` definition to 1.

All this can be tested with the following steps (commands from Ubuntu 20.04):
1. Compiling an example program, e.g. for ARM:
    echo 'int main() {puts("Hello world");}' | arm-linux-gnueabihf-gcc -xc -
2. Running qemu-arm with gdbstub:
    qemu-arm -g 1234 -L /usr/arm-linux-gnueabihf/ ./a.out
3. Connecting to the gdbstub with GDB:
    gdb-multiarch -ex 'target remote localhost:1234'
4. Executing `info proc mappings` in GDB.

The opening of files is done on behalf of the inferior by reusing the do_openat syscall.
Note that the current solution is still imperfect: while it allows us to fetch procfs
files like /proc/$pid/maps in the same way as the inferior is seeing them, there are
two downsides to it. First of all, it is indeed performed on behalf of the inferior.
Second of all, there are some files that the GDB tries to request like /lib/libc.so.6,
but they are usually not available as they do not exist in those paths and may need to
be served from the prefix provided in the -L flag to the qemu-user binary. I may try
to add a support for this in another patch and maybe refactor the solution to not use
the do_openat function directly.

Before this commit, one would get (except of amd64, but not i386 targets):
```
(gdb) info proc mappings
process 1
warning: unable to open /proc file '/proc/1/maps'
```

And after this commit, we should get something like:
```
(gdb) info proc mappings
process 3167519
Mapped address spaces:

	Start Addr   End Addr       Size     Offset objfile
	0x3f7d0000 0x3f7d1000     0x1000        0x0
	0x3f7d1000 0x3f7ed000    0x1c000        0x0 /usr/arm-linux-gnueabihf/lib/ld-2.33.so
	0x3f7ed000 0x3f7fd000    0x10000        0x0
	0x3f7fd000 0x3f7ff000     0x2000    0x1c000 /usr/arm-linux-gnueabihf/lib/ld-2.33.so
	0x3f7ff000 0x3f800000     0x1000        0x0
	0x3f800000 0x40000000   0x800000        0x0 [stack]
	0x40000000 0x40001000     0x1000        0x0 /home/dc/src/qemu/a.out
	0x40001000 0x40010000     0xf000        0x0
	0x40010000 0x40012000     0x2000        0x0 /home/dc/src/qemu/a.out
```

However, on amd64 targets we would get and still get the following on the GDB side
(even after this commit):
```
(gdb) info proc mappings
Not supported on this target.
```

The x64 behavior is related to the fact that the GDB client does not initialize
some of its remote handlers properly when the gdbstub does not send an "orig_rax"
register in the target.xml file that describes the target. This happens in GDB in the
amd64_linux_init_abi function in the amd64-linux-tdep.c file [0]. The GDB tries to find
the "org.gnu.gdb.i386.linux" feature and the "orig_rax" register in it and if it is not
present, then it does not proceed with the `amd64_linux_init_abi_common (info, gdbarch, 2);` call
which initializes whatever is needed so that GDB fetches `info proc mappings` properly.

I tried to fix this but just adding the orig_rax registry into the target.xml did not work
(there was some mismatch between the expected and sent register values; I guess the QEMU stub
would need to know how to send this register's value). On the other hand, this could also be
fixed on the GDB side. I will discuss this with GDB maintainers or/and propose a patch to GDB
related to this.

[0] https://github.com/bminor/binutils-gdb/blob/dc5483c989f29fc9c7934965071ae1bb80cff902/gdb/amd64-linux-tdep.c#L1863-L1873

Signed-off-by: Dominik 'Disconnect3d' Czarnota <dominik.b.czarnota@gmail.com>
---
 gdbstub.c            | 272 +++++++++++++++++++++++++++++++++++++++++++
 linux-user/qemu.h    |   2 +
 linux-user/syscall.c |   2 +-
 3 files changed, 275 insertions(+), 1 deletion(-)

ping

The patch can also be seen on patchew:
https://patchew.org/QEMU/20220221030910.3203063-1-dominik.b.czarnota@gmail.com/
or on lore.kernel.org:
https://lore.kernel.org/qemu-devel/20220221030910.3203063-1-dominik.b.czarnota@gmail.com/



On Mon, 21 Feb 2022 at 04:09, Disconnect3d <dominik.b.czarnota@gmail.com> wrote:
>
> This commit adds support for `info proc mappings` and a few other commands into
> the QEMU user-mode emulation gdbstub.
>
> For that, support for the following GDB remote protocol commands has been added:
> * vFile:setfs: pid
> * vFile:open: filename, flags, mode
> * vFile:pread: fd, count, offset
> * vFile:close: fd
> * qXfer:exec-file:read:annex:offset,length
>
> Additionally, a `REMOTE_DEBUG_PRINT` macro has been added for printing remote debug logs.
> To enable it, set the `ENABLE_REMOTE_DEBUG` definition to 1.
>
> All this can be tested with the following steps (commands from Ubuntu 20.04):
> 1. Compiling an example program, e.g. for ARM:
>     echo 'int main() {puts("Hello world");}' | arm-linux-gnueabihf-gcc -xc -
> 2. Running qemu-arm with gdbstub:
>     qemu-arm -g 1234 -L /usr/arm-linux-gnueabihf/ ./a.out
> 3. Connecting to the gdbstub with GDB:
>     gdb-multiarch -ex 'target remote localhost:1234'
> 4. Executing `info proc mappings` in GDB.
>
> The opening of files is done on behalf of the inferior by reusing the do_openat syscall.
> Note that the current solution is still imperfect: while it allows us to fetch procfs
> files like /proc/$pid/maps in the same way as the inferior is seeing them, there are
> two downsides to it. First of all, it is indeed performed on behalf of the inferior.
> Second of all, there are some files that the GDB tries to request like /lib/libc.so.6,
> but they are usually not available as they do not exist in those paths and may need to
> be served from the prefix provided in the -L flag to the qemu-user binary. I may try
> to add a support for this in another patch and maybe refactor the solution to not use
> the do_openat function directly.
>
> Before this commit, one would get (except of amd64, but not i386 targets):
> ```
> (gdb) info proc mappings
> process 1
> warning: unable to open /proc file '/proc/1/maps'
> ```
>
> And after this commit, we should get something like:
> ```
> (gdb) info proc mappings
> process 3167519
> Mapped address spaces:
>
>         Start Addr   End Addr       Size     Offset objfile
>         0x3f7d0000 0x3f7d1000     0x1000        0x0
>         0x3f7d1000 0x3f7ed000    0x1c000        0x0 /usr/arm-linux-gnueabihf/lib/ld-2.33.so
>         0x3f7ed000 0x3f7fd000    0x10000        0x0
>         0x3f7fd000 0x3f7ff000     0x2000    0x1c000 /usr/arm-linux-gnueabihf/lib/ld-2.33.so
>         0x3f7ff000 0x3f800000     0x1000        0x0
>         0x3f800000 0x40000000   0x800000        0x0 [stack]
>         0x40000000 0x40001000     0x1000        0x0 /home/dc/src/qemu/a.out
>         0x40001000 0x40010000     0xf000        0x0
>         0x40010000 0x40012000     0x2000        0x0 /home/dc/src/qemu/a.out
> ```
>
> However, on amd64 targets we would get and still get the following on the GDB side
> (even after this commit):
> ```
> (gdb) info proc mappings
> Not supported on this target.
> ```
>
> The x64 behavior is related to the fact that the GDB client does not initialize
> some of its remote handlers properly when the gdbstub does not send an "orig_rax"
> register in the target.xml file that describes the target. This happens in GDB in the
> amd64_linux_init_abi function in the amd64-linux-tdep.c file [0]. The GDB tries to find
> the "org.gnu.gdb.i386.linux" feature and the "orig_rax" register in it and if it is not
> present, then it does not proceed with the `amd64_linux_init_abi_common (info, gdbarch, 2);` call
> which initializes whatever is needed so that GDB fetches `info proc mappings` properly.
>
> I tried to fix this but just adding the orig_rax registry into the target.xml did not work
> (there was some mismatch between the expected and sent register values; I guess the QEMU stub
> would need to know how to send this register's value). On the other hand, this could also be
> fixed on the GDB side. I will discuss this with GDB maintainers or/and propose a patch to GDB
> related to this.
>
> [0] https://github.com/bminor/binutils-gdb/blob/dc5483c989f29fc9c7934965071ae1bb80cff902/gdb/amd64-linux-tdep.c#L1863-L1873
>
> Signed-off-by: Dominik 'Disconnect3d' Czarnota <dominik.b.czarnota@gmail.com>
> ---
>  gdbstub.c            | 272 +++++++++++++++++++++++++++++++++++++++++++
>  linux-user/qemu.h    |   2 +
>  linux-user/syscall.c |   2 +-
>  3 files changed, 275 insertions(+), 1 deletion(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 3c14c6a038..69cf8bbb0c 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -34,6 +34,10 @@
>  #include "exec/gdbstub.h"
>  #ifdef CONFIG_USER_ONLY
>  #include "qemu.h"
> +#ifdef CONFIG_LINUX
> +#include "linux-user/qemu.h"
> +#include "linux-user/loader.h"
> +#endif
>  #else
>  #include "monitor/monitor.h"
>  #include "chardev/char.h"
> @@ -62,6 +66,21 @@
>  static int phy_memory_mode;
>  #endif
>
> +/*
> + *  Set to 1 to enable remote protocol debugging output. This output is similar
> + *  to the one produced by the gdbserver's --remote-debug flag with some
> + *  additions. Anyway, the main debug prints are:
> + * - getpkt ("...") which refers to received data (or, send by the GDB client)
> + * - putpkt ("...") which refers to sent data
> + */
> +#define ENABLE_REMOTE_DEBUG 0
> +
> +#if ENABLE_REMOTE_DEBUG
> +#define REMOTE_DEBUG_PRINT printf
> +#else
> +#define REMOTE_DEBUG_PRINT(...)
> +#endif
> +
>  static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
>                                           uint8_t *buf, int len, bool is_write)
>  {
> @@ -554,6 +573,7 @@ static int gdb_continue_partial(char *newstates)
>
>  static void put_buffer(const uint8_t *buf, int len)
>  {
> +    REMOTE_DEBUG_PRINT("putpkt (\"%.*s\");\n", len, buf);
>  #ifdef CONFIG_USER_ONLY
>      int ret;
>
> @@ -1982,6 +2002,157 @@ static void handle_v_kill(GArray *params, void *user_ctx)
>      exit(0);
>  }
>
> +#ifdef CONFIG_USER_ONLY
> +/*
> + * Handles the `vFile:setfs: pid` command
> + *
> + * Example call: vFile:setfs:0
> + *
> + * --- From the GDB remote protocol documentation ---
> + * Select the filesystem on which vFile operations with filename arguments
> + * will operate. This is required for GDB to be able to access files on
> + * remote targets where the remote stub does not share a common filesystem with
> + * the inferior(s). If pid is nonzero, select the filesystem as seen by process
> + * pid. If pid is zero, select the filesystem as seen by the remote stub.
> + * Return 0 on success, or -1 if an error occurs. If vFile:setfs: indicates
> + * success, the selected filesystem remains selected until the next successful
> + * vFile:setfs: operation.
> +*/
> +static void handle_v_setfs(GArray *params, void *user_ctx)
> +{
> +    /*
> +     * We do not support different filesystem view for different pids
> +     * Return that all is OK, so that GDB can proceed
> +     */
> +    put_packet("F0");
> +}
> +
> +/*
> + * Handle the `vFile:open: filename, flags, mode` command
> + *
> + * We try to serve the filesystem here from the inferior point of view
> +
> + * Example call: vFile:open:6a7573742070726f62696e67,0,1c0
> + * (tries to open "just probing" with flags=0 mode=448)
> + *
> + * --- From the GDB remote protocol documentation ---
> + * Open a file at filename and return a file descriptor for it, or return
> + * -1 if an error occurs. The filename is a string, flags is an integer
> + * indicating a mask of open flags (see Open Flags), and mode is an integer
> + * indicating a mask of mode bits to use if the file is created
> + * (see mode_t Values). See open, for details of the open flags and mode
> + * values.
> + */
> +static void handle_v_file_open(GArray *params, void *user_ctx)
> +{
> +    uint64_t flags = get_param(params, 1)->val_ull;
> +    uint64_t mode = get_param(params, 2)->val_ull;
> +    const char *hex_filename = get_param(params, 0)->data;
> +
> +    /* Decode the filename & append a null byte so we can use it later on */
> +    hextomem(gdbserver_state.mem_buf, hex_filename, strlen(hex_filename));
> +    const char *null_byte = "\0";
> +    g_byte_array_append(gdbserver_state.mem_buf, (const guint8 *)null_byte, 1);
> +
> +    const char *filename = (const char *)gdbserver_state.mem_buf->data;
> +
> +    REMOTE_DEBUG_PRINT("vFile:open: filename=\"%s\" flags=%ld mode=%ld\n",
> +                       filename, flags, mode);
> +
> +    /*
> +     * On Linux we call the do_openat syscall on behalf of the inferior as it
> +     * handles special filepaths properly like the /proc/$pid files, which are
> +     * fetched by GDB for certain info (such as `info proc mappings`).
> +     */
> +#ifdef CONFIG_LINUX
> +    int fd = do_openat(gdbserver_state.g_cpu->env_ptr,
> +                       /* dirfd */ 0, filename, flags, mode);
> +    REMOTE_DEBUG_PRINT("do_openat = %d\n", fd);
> +#else
> +    int fd = open(filename, flags, mode);
> +    REMOTE_DEBUG_PRINT("open = %d\n", fd);
> +#endif
> +
> +    g_string_printf(gdbserver_state.str_buf, "F%d", fd);
> +    if (fd < 0) {
> +        /* Append ENOENT result.
> +         * TODO/FIXME: Can we retrieve errno from do_openat/open and return it here?
> +         */
> +        g_string_append(gdbserver_state.str_buf, ",2");
> +    }
> +    put_strbuf();
> +}
> +
> +/*
> + * Handles the `vFile:pread: fd, count, offset` command
> + *
> + * Example call: vFile:pread:7,47ff,0
> + *
> + * --- From the GDB remote protocol documentation ---
> + * Read data from the open file corresponding to fd.
> + * Up to count bytes will be read from the file, starting at offset relative to
> + * the start of the file. The target may read fewer bytes; common reasons
> + * include packet size limits and an end-of-file condition. The number of bytes
> + * read is returned. Zero should only be returned for a successful read at the
> + * end of the file, or if count was zero.
> + *
> + * The data read should be returned as a binary attachment on success. If zero
> + * bytes were read, the response should include an empty binary attachment
> + * (i.e. a trailing semicolon). The return value is the number of target bytes
> + * read; the binary attachment may be longer if some characters were escaped.
> + */
> +static void handle_v_file_pread(GArray *params, void *user_ctx)
> +{
> +    int fd = get_param(params, 0)->val_ul;
> +    uint64_t count = get_param(params, 1)->val_ull;
> +    uint64_t offset = get_param(params, 2)->val_ull;
> +
> +    g_autoptr(GString) file_content = g_string_new(NULL);
> +
> +    REMOTE_DEBUG_PRINT("vFile:read: fd=%d, count=%lu, offset=%lu\n",
> +                       fd, count, offset);
> +
> +    while (count > 0) {
> +        char buf[1024] = {0};
> +        ssize_t n = pread(fd, buf, sizeof(buf), offset);
> +        if (n <= 0) {
> +            break;
> +        }
> +        g_string_append_len(file_content, buf, n);
> +        count -= n;
> +        offset += n;
> +    }
> +    g_string_printf(gdbserver_state.str_buf, "F%lx;", file_content->len);
> +    /* Encode special chars */
> +    memtox(gdbserver_state.str_buf, file_content->str, file_content->len);
> +    put_packet_binary(gdbserver_state.str_buf->str,
> +                      gdbserver_state.str_buf->len, true);
> +}
> +
> +/*
> + * Handles the `vFile:close: fd` command
> + *
> + * Example call: vFile:close:7
> + *
> + * --- From the GDB remote protocol documentation ---
> + * Close the open file corresponding to fd and return 0, or -1 if an error occurs.
> + */
> +static void handle_v_file_close(GArray *params, void *user_ctx)
> +{
> +    int fd = get_param(params, 0)->val_ul;
> +    int res = close(fd);
> +    if (res == 0) {
> +        put_packet("F00");
> +    } else {
> +        /* This may happen only with a bugged GDB client or a bugged inferior */
> +        REMOTE_DEBUG_PRINT("Warning: the vFile:close(fd=%d) operation returned %d\n",
> +                           fd, res);
> +        g_string_printf(gdbserver_state.str_buf, "F%d,%d", res, errno);
> +        put_strbuf();
> +    }
> +}
> +#endif /* CONFIG_USER_ONLY */
> +
>  static const GdbCmdParseEntry gdb_v_commands_table[] = {
>      /* Order is important if has same prefix */
>      {
> @@ -2001,6 +2172,32 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
>          .cmd_startswith = 1,
>          .schema = "l0"
>      },
> +    #ifdef CONFIG_USER_ONLY
> +    {
> +        .handler = handle_v_setfs,
> +        .cmd = "File:setfs:",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
> +    {
> +        .handler = handle_v_file_open,
> +        .cmd = "File:open:",
> +        .cmd_startswith = 1,
> +        .schema = "s,L,L0"
> +    },
> +    {
> +        .handler = handle_v_file_pread,
> +        .cmd = "File:pread:",
> +        .cmd_startswith = 1,
> +        .schema = "l,L,L0"
> +    },
> +    {
> +        .handler = handle_v_file_close,
> +        .cmd = "File:close:",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
> +    #endif
>      {
>          .handler = handle_v_kill,
>          .cmd = "Kill;",
> @@ -2197,6 +2394,8 @@ static void handle_query_supported(GArray *params, void *user_ctx)
>      if (gdbserver_state.c_cpu->opaque) {
>          g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
>      }
> +
> +    g_string_append(gdbserver_state.str_buf, ";qXfer:exec-file:read+");
>  #endif
>
>      if (params->len &&
> @@ -2305,6 +2504,63 @@ static void handle_query_xfer_auxv(GArray *params, void *user_ctx)
>      put_packet_binary(gdbserver_state.str_buf->str,
>                        gdbserver_state.str_buf->len, true);
>  }
> +
> +/*
> + * Handle the `qXfer:exec-file:read:annex:offset,length` command
> + *
> + * Example call: qXfer:exec-file:read:241022:0,ffb
> + *
> + * --- From the GDB remote protocol documentation ---
> + * Return the full absolute name of the file that was executed to create a process
> + * running on the remote system. The annex specifies the numeric process ID of the
> + * process to query, encoded as a hexadecimal number. If the annex part is empty the
> + * remote stub should return the filename corresponding to the currently executing
> + * process.
> + *
> + * This packet is not probed by default; the remote stub must request it, by supplying
> + * an appropriate ‘qSupported’ response (see qSupported).
> + */
> +static void 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) {
> +        put_packet("E01");
> +        return;
> +    }
> +
> +    CPUState *cpu = get_first_cpu_in_process(process);
> +    if (!cpu) {
> +        put_packet("E02");
> +        return;
> +    }
> +
> +    TaskState *ts = cpu->opaque;
> +    /* Those should be there but lets sanity check them */
> +    if (!ts || !ts->bprm || !ts->bprm->filename) {
> +        put_packet("E03");
> +        return;
> +    }
> +
> +    /*
> +     * This filename is an absolute path even when QEMU user-mode emulation is called
> +     * with a symlink path so we do not have to resolve it with readlink(2)
> +     */
> +    const char *filename = ts->bprm->filename;
> +
> +    /* It does not make sense to return anything after the filename */
> +    if (offset > strlen(filename)) {
> +        put_packet("E04");
> +        return;
> +    }
> +
> +    g_string_printf(gdbserver_state.str_buf, "l%.*s", length, filename + offset);
> +    put_strbuf();
> +    return;
> +}
>  #endif
>
>  static void handle_query_attached(GArray *params, void *user_ctx)
> @@ -2419,6 +2675,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
>          .cmd_startswith = 1,
>          .schema = "l,l0"
>      },
> +    {
> +        .handler = handle_query_xfer_exec_file,
> +        .cmd = "Xfer:exec-file:read:",
> +        .cmd_startswith = 1,
> +        .schema = "l:l,l0"
> +    },
>  #endif
>      {
>          .handler = handle_query_attached,
> @@ -2516,6 +2778,7 @@ static int gdb_handle_packet(const char *line_buf)
>      const GdbCmdParseEntry *cmd_parser = NULL;
>
>      trace_gdbstub_io_command(line_buf);
> +    REMOTE_DEBUG_PRINT("getpkt (\"%s\");\n", line_buf);
>
>      switch (line_buf[0]) {
>      case '!':
> @@ -3133,6 +3396,15 @@ static void create_default_process(GDBState *s)
>      GDBProcess *process;
>      int max_pid = 0;
>
> +#if defined(CONFIG_USER_ONLY)
> +    /*
> +     * In QEMU user-mode emulation we want to return the real PID of the proces
> +     * as this allows us to return proper view of /proc/$pid files as seen by
> +     * the inferior
> +     */
> +    max_pid = getpid() - 1;
> +#endif
> +
>      if (gdbserver_state.process_num) {
>          max_pid = s->processes[s->process_num - 1].pid;
>      }
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 98dfbf2096..44a71b9740 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -182,6 +182,8 @@ static inline bool access_ok(CPUState *cpu, int type,
>      return access_ok_untagged(type, cpu_untagged_addr(cpu, addr), size);
>  }
>
> +int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, mode_t mode);
> +
>  /* NOTE __get_user and __put_user use host pointers and don't check access.
>     These are usually used to access struct data members once the struct has
>     been locked - usually with lock_user_struct.  */
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index b9b18a7eaf..bfc271b568 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8233,7 +8233,7 @@ static int open_hardware(void *cpu_env, int fd)
>  }
>  #endif
>
> -static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, mode_t mode)
> +int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, mode_t mode)
>  {
>      struct fake_open {
>          const char *filename;
> --
> 2.30.2
>

Disconnect3d <dominik.b.czarnota@gmail.com> writes:

> This commit adds support for `info proc mappings` and a few other commands into
> the QEMU user-mode emulation gdbstub.
>
> For that, support for the following GDB remote protocol commands has been added:
> * vFile:setfs: pid
> * vFile:open: filename, flags, mode
> * vFile:pread: fd, count, offset
> * vFile:close: fd
> * qXfer:exec-file:read:annex:offset,length
>
> Additionally, a `REMOTE_DEBUG_PRINT` macro has been added for printing remote debug logs.
> To enable it, set the `ENABLE_REMOTE_DEBUG` definition to 1.
>
> All this can be tested with the following steps (commands from Ubuntu 20.04):
> 1. Compiling an example program, e.g. for ARM:
>     echo 'int main() {puts("Hello world");}' | arm-linux-gnueabihf-gcc -xc -
> 2. Running qemu-arm with gdbstub:
>     qemu-arm -g 1234 -L /usr/arm-linux-gnueabihf/ ./a.out
> 3. Connecting to the gdbstub with GDB:
>     gdb-multiarch -ex 'target remote localhost:1234'
> 4. Executing `info proc mappings` in GDB.

It would be useful to add this to the check-tcg tests we do (there are
already other examples (see run-gdbstub-FOO tests in
tests/tcg/multiarch/Makefile.target)).

>
> The opening of files is done on behalf of the inferior by reusing the do_openat syscall.
> Note that the current solution is still imperfect: while it allows us to fetch procfs
> files like /proc/$pid/maps in the same way as the inferior is seeing them, there are
> two downsides to it. First of all, it is indeed performed on behalf of the inferior.
> Second of all, there are some files that the GDB tries to request like /lib/libc.so.6,
> but they are usually not available as they do not exist in those paths and may need to
> be served from the prefix provided in the -L flag to the qemu-user binary. I may try
> to add a support for this in another patch and maybe refactor the solution to not use
> the do_openat function directly.
>
> Before this commit, one would get (except of amd64, but not i386 targets):
>
> ```
> (gdb) info proc mappings
> process 1
> warning: unable to open /proc file '/proc/1/maps'
> ```
>
>
> And after this commit, we should get something like:
>
> ```
> (gdb) info proc mappings
> process 3167519
> Mapped address spaces:
>
> 	Start Addr   End Addr       Size     Offset objfile
> 	0x3f7d0000 0x3f7d1000     0x1000        0x0
> 	0x3f7d1000 0x3f7ed000    0x1c000        0x0 /usr/arm-linux-gnueabihf/lib/ld-2.33.so
> 	0x3f7ed000 0x3f7fd000    0x10000        0x0
> 	0x3f7fd000 0x3f7ff000     0x2000    0x1c000 /usr/arm-linux-gnueabihf/lib/ld-2.33.so
> 	0x3f7ff000 0x3f800000     0x1000        0x0
> 	0x3f800000 0x40000000   0x800000        0x0 [stack]
> 	0x40000000 0x40001000     0x1000        0x0 /home/dc/src/qemu/a.out
> 	0x40001000 0x40010000     0xf000        0x0
> 	0x40010000 0x40012000     0x2000        0x0 /home/dc/src/qemu/a.out
> ```
>
>
> However, on amd64 targets we would get and still get the following on the GDB side
> (even after this commit):
>
> ```
> (gdb) info proc mappings
> Not supported on this target.
> ```
>
> The x64 behavior is related to the fact that the GDB client does not initialize
> some of its remote handlers properly when the gdbstub does not send an "orig_rax"
> register in the target.xml file that describes the target. This happens in GDB in the
> amd64_linux_init_abi function in the amd64-linux-tdep.c file [0]. The GDB tries to find
> the "org.gnu.gdb.i386.linux" feature and the "orig_rax" register in it and if it is not
> present, then it does not proceed with the `amd64_linux_init_abi_common (info, gdbarch, 2);` call
> which initializes whatever is needed so that GDB fetches `info proc mappings` properly.
>
> I tried to fix this but just adding the orig_rax registry into the target.xml did not work
> (there was some mismatch between the expected and sent register values; I guess the QEMU stub
> would need to know how to send this register's value). On the other hand, this could also be
> fixed on the GDB side. I will discuss this with GDB maintainers or/and propose a patch to GDB
> related to this.

Yeah there is debate about sticking to the "official" XML for certain
arches vs just generating our own custom register XML which GDB doesn't
deal with the special cases.

>
> [0] https://github.com/bminor/binutils-gdb/blob/dc5483c989f29fc9c7934965071ae1bb80cff902/gdb/amd64-linux-tdep.c#L1863-L1873
>
> Signed-off-by: Dominik 'Disconnect3d' Czarnota <dominik.b.czarnota@gmail.com>
> ---
>  gdbstub.c            | 272 +++++++++++++++++++++++++++++++++++++++++++
>  linux-user/qemu.h    |   2 +
>  linux-user/syscall.c |   2 +-
>  3 files changed, 275 insertions(+), 1 deletion(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 3c14c6a038..69cf8bbb0c 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -34,6 +34,10 @@
>  #include "exec/gdbstub.h"
>  #ifdef CONFIG_USER_ONLY
>  #include "qemu.h"
> +#ifdef CONFIG_LINUX
> +#include "linux-user/qemu.h"
> +#include "linux-user/loader.h"
> +#endif
>  #else
>  #include "monitor/monitor.h"
>  #include "chardev/char.h"
> @@ -62,6 +66,21 @@
>  static int phy_memory_mode;
>  #endif
>  
> +/*
> + *  Set to 1 to enable remote protocol debugging output. This output is similar
> + *  to the one produced by the gdbserver's --remote-debug flag with some
> + *  additions. Anyway, the main debug prints are:
> + * - getpkt ("...") which refers to received data (or, send by the GDB client)
> + * - putpkt ("...") which refers to sent data
> + */
> +#define ENABLE_REMOTE_DEBUG 0
> +
> +#if ENABLE_REMOTE_DEBUG
> +#define REMOTE_DEBUG_PRINT printf
> +#else
> +#define REMOTE_DEBUG_PRINT(...)
> +#endif

We don't need this. The rest of the gdbstub has been instrumented with
tracepoints (try -d trace:gdbstub\* in your command line). See trace-events.

> +
>  static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
>                                           uint8_t *buf, int len, bool is_write)
>  {
> @@ -554,6 +573,7 @@ static int gdb_continue_partial(char *newstates)
>  
>  static void put_buffer(const uint8_t *buf, int len)
>  {
> +    REMOTE_DEBUG_PRINT("putpkt (\"%.*s\");\n", len, buf);
>  #ifdef CONFIG_USER_ONLY
>      int ret;
>  
> @@ -1982,6 +2002,157 @@ static void handle_v_kill(GArray *params, void *user_ctx)
>      exit(0);
>  }
>  
> +#ifdef CONFIG_USER_ONLY
> +/*
> + * Handles the `vFile:setfs: pid` command
> + *
> + * Example call: vFile:setfs:0
> + *
> + * --- From the GDB remote protocol documentation ---
> + * Select the filesystem on which vFile operations with filename arguments
> + * will operate. This is required for GDB to be able to access files on
> + * remote targets where the remote stub does not share a common filesystem with
> + * the inferior(s). If pid is nonzero, select the filesystem as seen by process
> + * pid. If pid is zero, select the filesystem as seen by the remote stub.
> + * Return 0 on success, or -1 if an error occurs. If vFile:setfs: indicates
> + * success, the selected filesystem remains selected until the next successful
> + * vFile:setfs: operation.
> +*/
> +static void handle_v_setfs(GArray *params, void *user_ctx)
> +{
> +    /*
> +     * We do not support different filesystem view for different pids
> +     * Return that all is OK, so that GDB can proceed
> +     */
> +    put_packet("F0");
> +}
> +
> +/*
> + * Handle the `vFile:open: filename, flags, mode` command
> + *
> + * We try to serve the filesystem here from the inferior point of view
> +
> + * Example call: vFile:open:6a7573742070726f62696e67,0,1c0
> + * (tries to open "just probing" with flags=0 mode=448)
> + *
> + * --- From the GDB remote protocol documentation ---
> + * Open a file at filename and return a file descriptor for it, or return
> + * -1 if an error occurs. The filename is a string, flags is an integer
> + * indicating a mask of open flags (see Open Flags), and mode is an integer
> + * indicating a mask of mode bits to use if the file is created
> + * (see mode_t Values). See open, for details of the open flags and mode
> + * values.
> + */
> +static void handle_v_file_open(GArray *params, void *user_ctx)
> +{
> +    uint64_t flags = get_param(params, 1)->val_ull;
> +    uint64_t mode = get_param(params, 2)->val_ull;
> +    const char *hex_filename = get_param(params, 0)->data;
> +
> +    /* Decode the filename & append a null byte so we can use it later on */
> +    hextomem(gdbserver_state.mem_buf, hex_filename, strlen(hex_filename));
> +    const char *null_byte = "\0";
> +    g_byte_array_append(gdbserver_state.mem_buf, (const guint8 *)null_byte, 1);
> +
> +    const char *filename = (const char *)gdbserver_state.mem_buf->data;
> +
> +    REMOTE_DEBUG_PRINT("vFile:open: filename=\"%s\" flags=%ld mode=%ld\n",
> +                       filename, flags, mode);
> +
> +    /*
> +     * On Linux we call the do_openat syscall on behalf of the inferior as it
> +     * handles special filepaths properly like the /proc/$pid files, which are
> +     * fetched by GDB for certain info (such as `info proc mappings`).
> +     */

This sounds like a massive security hole to introduce without a
specific flag the user can explicitly enable. Semihosting has a similar
facility but also needs explicit configuration. Can we add a property to
the -gdb command line option to enable this:

  -gdb tcp::1234,hostio=on

?


> +#ifdef CONFIG_LINUX
> +    int fd = do_openat(gdbserver_state.g_cpu->env_ptr,
> +                       /* dirfd */ 0, filename, flags, mode);



> +    REMOTE_DEBUG_PRINT("do_openat = %d\n", fd);
> +#else
> +    int fd = open(filename, flags, mode);
> +    REMOTE_DEBUG_PRINT("open = %d\n", fd);
> +#endif
> +
> +    g_string_printf(gdbserver_state.str_buf, "F%d", fd);
> +    if (fd < 0) {
> +        /* Append ENOENT result.
> +         * TODO/FIXME: Can we retrieve errno from do_openat/open and return it here?
> +         */
> +        g_string_append(gdbserver_state.str_buf, ",2");
> +    }
> +    put_strbuf();
> +}
> +
> +/*
> + * Handles the `vFile:pread: fd, count, offset` command
> + *
> + * Example call: vFile:pread:7,47ff,0
> + *
> + * --- From the GDB remote protocol documentation ---
> + * Read data from the open file corresponding to fd.
> + * Up to count bytes will be read from the file, starting at offset relative to
> + * the start of the file. The target may read fewer bytes; common reasons
> + * include packet size limits and an end-of-file condition. The number of bytes
> + * read is returned. Zero should only be returned for a successful read at the
> + * end of the file, or if count was zero.
> + *
> + * The data read should be returned as a binary attachment on success. If zero
> + * bytes were read, the response should include an empty binary attachment
> + * (i.e. a trailing semicolon). The return value is the number of target bytes
> + * read; the binary attachment may be longer if some characters were escaped.
> + */
> +static void handle_v_file_pread(GArray *params, void *user_ctx)
> +{
> +    int fd = get_param(params, 0)->val_ul;
> +    uint64_t count = get_param(params, 1)->val_ull;
> +    uint64_t offset = get_param(params, 2)->val_ull;
> +
> +    g_autoptr(GString) file_content = g_string_new(NULL);
> +
> +    REMOTE_DEBUG_PRINT("vFile:read: fd=%d, count=%lu, offset=%lu\n",
> +                       fd, count, offset);
> +
> +    while (count > 0) {
> +        char buf[1024] = {0};
> +        ssize_t n = pread(fd, buf, sizeof(buf), offset);

I think for this size of buffer I'd rather allocate it on the heap.

> +        if (n <= 0) {
> +            break;
> +        }
> +        g_string_append_len(file_content, buf, n);
> +        count -= n;
> +        offset += n;
> +    }
> +    g_string_printf(gdbserver_state.str_buf, "F%lx;", file_content->len);
> +    /* Encode special chars */
> +    memtox(gdbserver_state.str_buf, file_content->str, file_content->len);
> +    put_packet_binary(gdbserver_state.str_buf->str,
> +                      gdbserver_state.str_buf->len, true);
> +}
> +
> +/*
> + * Handles the `vFile:close: fd` command
> + *
> + * Example call: vFile:close:7
> + *
> + * --- From the GDB remote protocol documentation ---
> + * Close the open file corresponding to fd and return 0, or -1 if an error occurs.
> + */
> +static void handle_v_file_close(GArray *params, void *user_ctx)
> +{
> +    int fd = get_param(params, 0)->val_ul;
> +    int res = close(fd);
> +    if (res == 0) {
> +        put_packet("F00");
> +    } else {
> +        /* This may happen only with a bugged GDB client or a bugged inferior */
> +        REMOTE_DEBUG_PRINT("Warning: the vFile:close(fd=%d) operation returned %d\n",
> +                           fd, res);

If you really wanted this could be qemu_log_mask(LOG_GUEST_ERROR, ...)

> +        g_string_printf(gdbserver_state.str_buf, "F%d,%d", res, errno);
> +        put_strbuf();
> +    }
> +}
> +#endif /* CONFIG_USER_ONLY */
> +
>  static const GdbCmdParseEntry gdb_v_commands_table[] = {
>      /* Order is important if has same prefix */
>      {
> @@ -2001,6 +2172,32 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
>          .cmd_startswith = 1,
>          .schema = "l0"
>      },
> +    #ifdef CONFIG_USER_ONLY
> +    {
> +        .handler = handle_v_setfs,
> +        .cmd = "File:setfs:",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
> +    {
> +        .handler = handle_v_file_open,
> +        .cmd = "File:open:",
> +        .cmd_startswith = 1,
> +        .schema = "s,L,L0"
> +    },
> +    {
> +        .handler = handle_v_file_pread,
> +        .cmd = "File:pread:",
> +        .cmd_startswith = 1,
> +        .schema = "l,L,L0"
> +    },
> +    {
> +        .handler = handle_v_file_close,
> +        .cmd = "File:close:",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
> +    #endif
>      {
>          .handler = handle_v_kill,
>          .cmd = "Kill;",
> @@ -2197,6 +2394,8 @@ static void handle_query_supported(GArray *params, void *user_ctx)
>      if (gdbserver_state.c_cpu->opaque) {
>          g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
>      }
> +
> +    g_string_append(gdbserver_state.str_buf, ";qXfer:exec-file:read+");
>  #endif
>  
>      if (params->len &&
> @@ -2305,6 +2504,63 @@ static void handle_query_xfer_auxv(GArray *params, void *user_ctx)
>      put_packet_binary(gdbserver_state.str_buf->str,
>                        gdbserver_state.str_buf->len, true);
>  }
> +
> +/*
> + * Handle the `qXfer:exec-file:read:annex:offset,length` command
> + *
> + * Example call: qXfer:exec-file:read:241022:0,ffb
> + *
> + * --- From the GDB remote protocol documentation ---
> + * Return the full absolute name of the file that was executed to create a process
> + * running on the remote system. The annex specifies the numeric process ID of the
> + * process to query, encoded as a hexadecimal number. If the annex part is empty the
> + * remote stub should return the filename corresponding to the currently executing
> + * process.
> + *
> + * This packet is not probed by default; the remote stub must request it, by supplying
> + * an appropriate ‘qSupported’ response (see qSupported).
> + */
> +static void 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) {
> +        put_packet("E01");
> +        return;
> +    }
> +
> +    CPUState *cpu = get_first_cpu_in_process(process);
> +    if (!cpu) {
> +        put_packet("E02");
> +        return;
> +    }
> +
> +    TaskState *ts = cpu->opaque;
> +    /* Those should be there but lets sanity check them */
> +    if (!ts || !ts->bprm || !ts->bprm->filename) {
> +        put_packet("E03");
> +        return;
> +    }
> +
> +    /*
> +     * This filename is an absolute path even when QEMU user-mode emulation is called
> +     * with a symlink path so we do not have to resolve it with readlink(2)
> +     */
> +    const char *filename = ts->bprm->filename;
> +
> +    /* It does not make sense to return anything after the filename */
> +    if (offset > strlen(filename)) {
> +        put_packet("E04");
> +        return;
> +    }
> +
> +    g_string_printf(gdbserver_state.str_buf, "l%.*s", length, filename + offset);
> +    put_strbuf();
> +    return;
> +}
>  #endif
>  
>  static void handle_query_attached(GArray *params, void *user_ctx)
> @@ -2419,6 +2675,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
>          .cmd_startswith = 1,
>          .schema = "l,l0"
>      },
> +    {
> +        .handler = handle_query_xfer_exec_file,
> +        .cmd = "Xfer:exec-file:read:",
> +        .cmd_startswith = 1,
> +        .schema = "l:l,l0"
> +    },
>  #endif
>      {
>          .handler = handle_query_attached,
> @@ -2516,6 +2778,7 @@ static int gdb_handle_packet(const char *line_buf)
>      const GdbCmdParseEntry *cmd_parser = NULL;
>  
>      trace_gdbstub_io_command(line_buf);
> +    REMOTE_DEBUG_PRINT("getpkt (\"%s\");\n", line_buf);
>  
>      switch (line_buf[0]) {
>      case '!':
> @@ -3133,6 +3396,15 @@ static void create_default_process(GDBState *s)
>      GDBProcess *process;
>      int max_pid = 0;
>  
> +#if defined(CONFIG_USER_ONLY)
> +    /*
> +     * In QEMU user-mode emulation we want to return the real PID of the proces
> +     * as this allows us to return proper view of /proc/$pid files as seen by
> +     * the inferior
> +     */
> +    max_pid = getpid() - 1;
> +#endif
> +
>      if (gdbserver_state.process_num) {
>          max_pid = s->processes[s->process_num - 1].pid;
>      }
> diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> index 98dfbf2096..44a71b9740 100644
> --- a/linux-user/qemu.h
> +++ b/linux-user/qemu.h
> @@ -182,6 +182,8 @@ static inline bool access_ok(CPUState *cpu, int type,
>      return access_ok_untagged(type, cpu_untagged_addr(cpu, addr), size);
>  }
>  
> +int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, mode_t mode);
> +
>  /* NOTE __get_user and __put_user use host pointers and don't check access.
>     These are usually used to access struct data members once the struct has
>     been locked - usually with lock_user_struct.  */
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index b9b18a7eaf..bfc271b568 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8233,7 +8233,7 @@ static int open_hardware(void *cpu_env, int fd)
>  }
>  #endif
>  
> -static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, mode_t mode)
> +int do_openat(void *cpu_env, int dirfd, const char *pathname, int
> flags, mode_t mode)


I wonder if this should be renamed to make the sense more clear?

do_guest_openat? do_filtered_openat?

>  {
>      struct fake_open {
>          const char *filename;

Otherwise it looks fine. Do you have time to re-spin? I would need to
post a pull request with it by the 8th to make 7.0.

Hey,

I may work on this next week but I will probably not make it until the 8th :(.

On Thu, 3 Mar 2022 at 13:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Disconnect3d <dominik.b.czarnota@gmail.com> writes:
>
> > This commit adds support for `info proc mappings` and a few other commands into
> > the QEMU user-mode emulation gdbstub.
> >
> > For that, support for the following GDB remote protocol commands has been added:
> > * vFile:setfs: pid
> > * vFile:open: filename, flags, mode
> > * vFile:pread: fd, count, offset
> > * vFile:close: fd
> > * qXfer:exec-file:read:annex:offset,length
> >
> > Additionally, a `REMOTE_DEBUG_PRINT` macro has been added for printing remote debug logs.
> > To enable it, set the `ENABLE_REMOTE_DEBUG` definition to 1.
> >
> > All this can be tested with the following steps (commands from Ubuntu 20.04):
> > 1. Compiling an example program, e.g. for ARM:
> >     echo 'int main() {puts("Hello world");}' | arm-linux-gnueabihf-gcc -xc -
> > 2. Running qemu-arm with gdbstub:
> >     qemu-arm -g 1234 -L /usr/arm-linux-gnueabihf/ ./a.out
> > 3. Connecting to the gdbstub with GDB:
> >     gdb-multiarch -ex 'target remote localhost:1234'
> > 4. Executing `info proc mappings` in GDB.
>
> It would be useful to add this to the check-tcg tests we do (there are
> already other examples (see run-gdbstub-FOO tests in
> tests/tcg/multiarch/Makefile.target)).
>

Thanks, I will check it out.

> >
> > The opening of files is done on behalf of the inferior by reusing the do_openat syscall.
> > Note that the current solution is still imperfect: while it allows us to fetch procfs
> > files like /proc/$pid/maps in the same way as the inferior is seeing them, there are
> > two downsides to it. First of all, it is indeed performed on behalf of the inferior.
> > Second of all, there are some files that the GDB tries to request like /lib/libc.so.6,
> > but they are usually not available as they do not exist in those paths and may need to
> > be served from the prefix provided in the -L flag to the qemu-user binary. I may try
> > to add a support for this in another patch and maybe refactor the solution to not use
> > the do_openat function directly.
> >
> > Before this commit, one would get (except of amd64, but not i386 targets):
> >
> > ```
> > (gdb) info proc mappings
> > process 1
> > warning: unable to open /proc file '/proc/1/maps'
> > ```
> >
> >
> > And after this commit, we should get something like:
> >
> > ```
> > (gdb) info proc mappings
> > process 3167519
> > Mapped address spaces:
> >
> >       Start Addr   End Addr       Size     Offset objfile
> >       0x3f7d0000 0x3f7d1000     0x1000        0x0
> >       0x3f7d1000 0x3f7ed000    0x1c000        0x0 /usr/arm-linux-gnueabihf/lib/ld-2.33.so
> >       0x3f7ed000 0x3f7fd000    0x10000        0x0
> >       0x3f7fd000 0x3f7ff000     0x2000    0x1c000 /usr/arm-linux-gnueabihf/lib/ld-2.33.so
> >       0x3f7ff000 0x3f800000     0x1000        0x0
> >       0x3f800000 0x40000000   0x800000        0x0 [stack]
> >       0x40000000 0x40001000     0x1000        0x0 /home/dc/src/qemu/a.out
> >       0x40001000 0x40010000     0xf000        0x0
> >       0x40010000 0x40012000     0x2000        0x0 /home/dc/src/qemu/a.out
> > ```
> >
> >
> > However, on amd64 targets we would get and still get the following on the GDB side
> > (even after this commit):
> >
> > ```
> > (gdb) info proc mappings
> > Not supported on this target.
> > ```
> >
> > The x64 behavior is related to the fact that the GDB client does not initialize
> > some of its remote handlers properly when the gdbstub does not send an "orig_rax"
> > register in the target.xml file that describes the target. This happens in GDB in the
> > amd64_linux_init_abi function in the amd64-linux-tdep.c file [0]. The GDB tries to find
> > the "org.gnu.gdb.i386.linux" feature and the "orig_rax" register in it and if it is not
> > present, then it does not proceed with the `amd64_linux_init_abi_common (info, gdbarch, 2);` call
> > which initializes whatever is needed so that GDB fetches `info proc mappings` properly.
> >
> > I tried to fix this but just adding the orig_rax registry into the target.xml did not work
> > (there was some mismatch between the expected and sent register values; I guess the QEMU stub
> > would need to know how to send this register's value). On the other hand, this could also be
> > fixed on the GDB side. I will discuss this with GDB maintainers or/and propose a patch to GDB
> > related to this.
>
> Yeah there is debate about sticking to the "official" XML for certain
> arches vs just generating our own custom register XML which GDB doesn't
> deal with the special cases.
>
> >
> > [0] https://github.com/bminor/binutils-gdb/blob/dc5483c989f29fc9c7934965071ae1bb80cff902/gdb/amd64-linux-tdep.c#L1863-L1873
> >
> > Signed-off-by: Dominik 'Disconnect3d' Czarnota <dominik.b.czarnota@gmail.com>
> > ---
> >  gdbstub.c            | 272 +++++++++++++++++++++++++++++++++++++++++++
> >  linux-user/qemu.h    |   2 +
> >  linux-user/syscall.c |   2 +-
> >  3 files changed, 275 insertions(+), 1 deletion(-)
> >
> > diff --git a/gdbstub.c b/gdbstub.c
> > index 3c14c6a038..69cf8bbb0c 100644
> > --- a/gdbstub.c
> > +++ b/gdbstub.c
> > @@ -34,6 +34,10 @@
> >  #include "exec/gdbstub.h"
> >  #ifdef CONFIG_USER_ONLY
> >  #include "qemu.h"
> > +#ifdef CONFIG_LINUX
> > +#include "linux-user/qemu.h"
> > +#include "linux-user/loader.h"
> > +#endif
> >  #else
> >  #include "monitor/monitor.h"
> >  #include "chardev/char.h"
> > @@ -62,6 +66,21 @@
> >  static int phy_memory_mode;
> >  #endif
> >
> > +/*
> > + *  Set to 1 to enable remote protocol debugging output. This output is similar
> > + *  to the one produced by the gdbserver's --remote-debug flag with some
> > + *  additions. Anyway, the main debug prints are:
> > + * - getpkt ("...") which refers to received data (or, send by the GDB client)
> > + * - putpkt ("...") which refers to sent data
> > + */
> > +#define ENABLE_REMOTE_DEBUG 0
> > +
> > +#if ENABLE_REMOTE_DEBUG
> > +#define REMOTE_DEBUG_PRINT printf
> > +#else
> > +#define REMOTE_DEBUG_PRINT(...)
> > +#endif
>
> We don't need this. The rest of the gdbstub has been instrumented with
> tracepoints (try -d trace:gdbstub\* in your command line). See trace-events.
>

It would be convenient to support the gddbserver's --remote-debug flag
but I understand it may not be wanted as it also means additional &
unnecessary code in a production binary.
I will remove this.

> > +
> >  static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
> >                                           uint8_t *buf, int len, bool is_write)
> >  {
> > @@ -554,6 +573,7 @@ static int gdb_continue_partial(char *newstates)
> >
> >  static void put_buffer(const uint8_t *buf, int len)
> >  {
> > +    REMOTE_DEBUG_PRINT("putpkt (\"%.*s\");\n", len, buf);
> >  #ifdef CONFIG_USER_ONLY
> >      int ret;
> >
> > @@ -1982,6 +2002,157 @@ static void handle_v_kill(GArray *params, void *user_ctx)
> >      exit(0);
> >  }
> >
> > +#ifdef CONFIG_USER_ONLY
> > +/*
> > + * Handles the `vFile:setfs: pid` command
> > + *
> > + * Example call: vFile:setfs:0
> > + *
> > + * --- From the GDB remote protocol documentation ---
> > + * Select the filesystem on which vFile operations with filename arguments
> > + * will operate. This is required for GDB to be able to access files on
> > + * remote targets where the remote stub does not share a common filesystem with
> > + * the inferior(s). If pid is nonzero, select the filesystem as seen by process
> > + * pid. If pid is zero, select the filesystem as seen by the remote stub.
> > + * Return 0 on success, or -1 if an error occurs. If vFile:setfs: indicates
> > + * success, the selected filesystem remains selected until the next successful
> > + * vFile:setfs: operation.
> > +*/
> > +static void handle_v_setfs(GArray *params, void *user_ctx)
> > +{
> > +    /*
> > +     * We do not support different filesystem view for different pids
> > +     * Return that all is OK, so that GDB can proceed
> > +     */
> > +    put_packet("F0");
> > +}
> > +
> > +/*
> > + * Handle the `vFile:open: filename, flags, mode` command
> > + *
> > + * We try to serve the filesystem here from the inferior point of view
> > +
> > + * Example call: vFile:open:6a7573742070726f62696e67,0,1c0
> > + * (tries to open "just probing" with flags=0 mode=448)
> > + *
> > + * --- From the GDB remote protocol documentation ---
> > + * Open a file at filename and return a file descriptor for it, or return
> > + * -1 if an error occurs. The filename is a string, flags is an integer
> > + * indicating a mask of open flags (see Open Flags), and mode is an integer
> > + * indicating a mask of mode bits to use if the file is created
> > + * (see mode_t Values). See open, for details of the open flags and mode
> > + * values.
> > + */
> > +static void handle_v_file_open(GArray *params, void *user_ctx)
> > +{
> > +    uint64_t flags = get_param(params, 1)->val_ull;
> > +    uint64_t mode = get_param(params, 2)->val_ull;
> > +    const char *hex_filename = get_param(params, 0)->data;
> > +
> > +    /* Decode the filename & append a null byte so we can use it later on */
> > +    hextomem(gdbserver_state.mem_buf, hex_filename, strlen(hex_filename));
> > +    const char *null_byte = "\0";
> > +    g_byte_array_append(gdbserver_state.mem_buf, (const guint8 *)null_byte, 1);
> > +
> > +    const char *filename = (const char *)gdbserver_state.mem_buf->data;
> > +
> > +    REMOTE_DEBUG_PRINT("vFile:open: filename=\"%s\" flags=%ld mode=%ld\n",
> > +                       filename, flags, mode);
> > +
> > +    /*
> > +     * On Linux we call the do_openat syscall on behalf of the inferior as it
> > +     * handles special filepaths properly like the /proc/$pid files, which are
> > +     * fetched by GDB for certain info (such as `info proc mappings`).
> > +     */
>
> This sounds like a massive security hole to introduce without a
> specific flag the user can explicitly enable. Semihosting has a similar
> facility but also needs explicit configuration. Can we add a property to
> the -gdb command line option to enable this:
>
>   -gdb tcp::1234,hostio=on
>
> ?

It is, but should we really care? Both the execution and debugging of
qemu-user emulation
does not protect the host from arbitrary code execution and the
proposed hostio=on flag will
only make things less convenient and many people will just not use it
due to not knowing that
it exists.

The program below shows an example that the qemu-user emulation
doesn't protect the host at all:
```
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#define QEMU_BIN_PATH "/usr/bin/qemu-x86_64"

char* find_qemu_map(FILE* fp) {
    ssize_t read = 0;
    size_t len = 0;
    char* line = NULL;
    while ((read = getline(&line, &len, fp)) != -1) {
        printf("[maps] read line (%lu) = %s\n", len, line);
        if (strstr(line, QEMU_BIN_PATH) != NULL) {
            return line;
        }
    }
    return NULL;
}

#define ENSURE(cond) if (!(cond)) { printf("%d: !" #cond "\n",
__LINE__); exit(1); }

int main() {
    char cat_maps[128] = {0};
    sprintf(cat_maps, "/bin/cat /proc/%d/maps", getpid());
    printf("Will read maps from %s\n", cat_maps);
    FILE* fp = popen(cat_maps, "r");
    ENSURE(fp);

    char* line = find_qemu_map(fp);
    ENSURE(line);

/* Examples:
555555554000-555555701000 r-xp 00000000 fc:01 667422
  /usr/bin/qemu-x86_64
555555901000-555555934000 r--p 001ad000 fc:01 667422
  /usr/bin/qemu-x86_64
555555934000-555555940000 rw-p 001e0000 fc:01 667422
  /usr/bin/qemu-x86_64
*/
    void* qemu_base;
    sscanf(line, "%llx-", (unsigned long long*)&qemu_base);
    printf("qemu base = %p\n", qemu_base);
    free(line); // getline allocates line in find_qemu_map
    fclose(fp);
}
```

If you run it, you will see that the debugged process can access the
original maps of the qemu
(and not debugged) process by spawning a cat process that will be
executed out of the emulation.

In a similar way, the emulated process can then write to
/proc/$pid/mem to overwrite the QEMU's
process executable memory to execute arbitrary code (outside of
emulated context).
Actually, for this the emulated process does not even need to use
popen as I believe the current
do_openat implementation does not fake the /proc/$pid/mem file at all.

Of course the same could be done in GDB, as the GDB client could
invoke commands to overwrite the
emulated process code to trigger the same actions as described above.

To sum up, a security hole already exists in the QEMU user emulation
which is that the emulated
process can spawn any other processes and that not all of the /proc/
paths are emulated. Given this
I am not sure if it is worth introducing a new flag just because we
want to add a feature that may likely be
expected to work as-is by GDB<->QEMU users.

On the other hand it's generally bad that GDB fetches memory maps
information by requesting a file from
the gdbstub and then parsing it. It would be great to have a command
that returns only this data and also
in a format that could be deserialized 'as-is' to the native GDB
structures (but then, different architectures,
endianess etc would be problematic).

>
>
> > +#ifdef CONFIG_LINUX
> > +    int fd = do_openat(gdbserver_state.g_cpu->env_ptr,
> > +                       /* dirfd */ 0, filename, flags, mode);
>
>
>
> > +    REMOTE_DEBUG_PRINT("do_openat = %d\n", fd);
> > +#else
> > +    int fd = open(filename, flags, mode);
> > +    REMOTE_DEBUG_PRINT("open = %d\n", fd);
> > +#endif
> > +
> > +    g_string_printf(gdbserver_state.str_buf, "F%d", fd);
> > +    if (fd < 0) {
> > +        /* Append ENOENT result.
> > +         * TODO/FIXME: Can we retrieve errno from do_openat/open and return it here?
> > +         */
> > +        g_string_append(gdbserver_state.str_buf, ",2");
> > +    }
> > +    put_strbuf();
> > +}
> > +
> > +/*
> > + * Handles the `vFile:pread: fd, count, offset` command
> > + *
> > + * Example call: vFile:pread:7,47ff,0
> > + *
> > + * --- From the GDB remote protocol documentation ---
> > + * Read data from the open file corresponding to fd.
> > + * Up to count bytes will be read from the file, starting at offset relative to
> > + * the start of the file. The target may read fewer bytes; common reasons
> > + * include packet size limits and an end-of-file condition. The number of bytes
> > + * read is returned. Zero should only be returned for a successful read at the
> > + * end of the file, or if count was zero.
> > + *
> > + * The data read should be returned as a binary attachment on success. If zero
> > + * bytes were read, the response should include an empty binary attachment
> > + * (i.e. a trailing semicolon). The return value is the number of target bytes
> > + * read; the binary attachment may be longer if some characters were escaped.
> > + */
> > +static void handle_v_file_pread(GArray *params, void *user_ctx)
> > +{
> > +    int fd = get_param(params, 0)->val_ul;
> > +    uint64_t count = get_param(params, 1)->val_ull;
> > +    uint64_t offset = get_param(params, 2)->val_ull;
> > +
> > +    g_autoptr(GString) file_content = g_string_new(NULL);
> > +
> > +    REMOTE_DEBUG_PRINT("vFile:read: fd=%d, count=%lu, offset=%lu\n",
> > +                       fd, count, offset);
> > +
> > +    while (count > 0) {
> > +        char buf[1024] = {0};
> > +        ssize_t n = pread(fd, buf, sizeof(buf), offset);
>
> I think for this size of buffer I'd rather allocate it on the heap.
>

+1

> > +        if (n <= 0) {
> > +            break;
> > +        }
> > +        g_string_append_len(file_content, buf, n);
> > +        count -= n;
> > +        offset += n;
> > +    }
> > +    g_string_printf(gdbserver_state.str_buf, "F%lx;", file_content->len);
> > +    /* Encode special chars */
> > +    memtox(gdbserver_state.str_buf, file_content->str, file_content->len);
> > +    put_packet_binary(gdbserver_state.str_buf->str,
> > +                      gdbserver_state.str_buf->len, true);
> > +}
> > +
> > +/*
> > + * Handles the `vFile:close: fd` command
> > + *
> > + * Example call: vFile:close:7
> > + *
> > + * --- From the GDB remote protocol documentation ---
> > + * Close the open file corresponding to fd and return 0, or -1 if an error occurs.
> > + */
> > +static void handle_v_file_close(GArray *params, void *user_ctx)
> > +{
> > +    int fd = get_param(params, 0)->val_ul;
> > +    int res = close(fd);
> > +    if (res == 0) {
> > +        put_packet("F00");
> > +    } else {
> > +        /* This may happen only with a bugged GDB client or a bugged inferior */
> > +        REMOTE_DEBUG_PRINT("Warning: the vFile:close(fd=%d) operation returned %d\n",
> > +                           fd, res);
>
> If you really wanted this could be qemu_log_mask(LOG_GUEST_ERROR, ...)
>

+1

> > +        g_string_printf(gdbserver_state.str_buf, "F%d,%d", res, errno);
> > +        put_strbuf();
> > +    }
> > +}
> > +#endif /* CONFIG_USER_ONLY */
> > +
> >  static const GdbCmdParseEntry gdb_v_commands_table[] = {
> >      /* Order is important if has same prefix */
> >      {
> > @@ -2001,6 +2172,32 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
> >          .cmd_startswith = 1,
> >          .schema = "l0"
> >      },
> > +    #ifdef CONFIG_USER_ONLY
> > +    {
> > +        .handler = handle_v_setfs,
> > +        .cmd = "File:setfs:",
> > +        .cmd_startswith = 1,
> > +        .schema = "l0"
> > +    },
> > +    {
> > +        .handler = handle_v_file_open,
> > +        .cmd = "File:open:",
> > +        .cmd_startswith = 1,
> > +        .schema = "s,L,L0"
> > +    },
> > +    {
> > +        .handler = handle_v_file_pread,
> > +        .cmd = "File:pread:",
> > +        .cmd_startswith = 1,
> > +        .schema = "l,L,L0"
> > +    },
> > +    {
> > +        .handler = handle_v_file_close,
> > +        .cmd = "File:close:",
> > +        .cmd_startswith = 1,
> > +        .schema = "l0"
> > +    },
> > +    #endif
> >      {
> >          .handler = handle_v_kill,
> >          .cmd = "Kill;",
> > @@ -2197,6 +2394,8 @@ static void handle_query_supported(GArray *params, void *user_ctx)
> >      if (gdbserver_state.c_cpu->opaque) {
> >          g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
> >      }
> > +
> > +    g_string_append(gdbserver_state.str_buf, ";qXfer:exec-file:read+");
> >  #endif
> >
> >      if (params->len &&
> > @@ -2305,6 +2504,63 @@ static void handle_query_xfer_auxv(GArray *params, void *user_ctx)
> >      put_packet_binary(gdbserver_state.str_buf->str,
> >                        gdbserver_state.str_buf->len, true);
> >  }
> > +
> > +/*
> > + * Handle the `qXfer:exec-file:read:annex:offset,length` command
> > + *
> > + * Example call: qXfer:exec-file:read:241022:0,ffb
> > + *
> > + * --- From the GDB remote protocol documentation ---
> > + * Return the full absolute name of the file that was executed to create a process
> > + * running on the remote system. The annex specifies the numeric process ID of the
> > + * process to query, encoded as a hexadecimal number. If the annex part is empty the
> > + * remote stub should return the filename corresponding to the currently executing
> > + * process.
> > + *
> > + * This packet is not probed by default; the remote stub must request it, by supplying
> > + * an appropriate ‘qSupported’ response (see qSupported).
> > + */
> > +static void 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) {
> > +        put_packet("E01");
> > +        return;
> > +    }
> > +
> > +    CPUState *cpu = get_first_cpu_in_process(process);
> > +    if (!cpu) {
> > +        put_packet("E02");
> > +        return;
> > +    }
> > +
> > +    TaskState *ts = cpu->opaque;
> > +    /* Those should be there but lets sanity check them */
> > +    if (!ts || !ts->bprm || !ts->bprm->filename) {
> > +        put_packet("E03");
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * This filename is an absolute path even when QEMU user-mode emulation is called
> > +     * with a symlink path so we do not have to resolve it with readlink(2)
> > +     */
> > +    const char *filename = ts->bprm->filename;
> > +
> > +    /* It does not make sense to return anything after the filename */
> > +    if (offset > strlen(filename)) {
> > +        put_packet("E04");
> > +        return;
> > +    }
> > +
> > +    g_string_printf(gdbserver_state.str_buf, "l%.*s", length, filename + offset);
> > +    put_strbuf();
> > +    return;
> > +}
> >  #endif
> >
> >  static void handle_query_attached(GArray *params, void *user_ctx)
> > @@ -2419,6 +2675,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
> >          .cmd_startswith = 1,
> >          .schema = "l,l0"
> >      },
> > +    {
> > +        .handler = handle_query_xfer_exec_file,
> > +        .cmd = "Xfer:exec-file:read:",
> > +        .cmd_startswith = 1,
> > +        .schema = "l:l,l0"
> > +    },
> >  #endif
> >      {
> >          .handler = handle_query_attached,
> > @@ -2516,6 +2778,7 @@ static int gdb_handle_packet(const char *line_buf)
> >      const GdbCmdParseEntry *cmd_parser = NULL;
> >
> >      trace_gdbstub_io_command(line_buf);
> > +    REMOTE_DEBUG_PRINT("getpkt (\"%s\");\n", line_buf);
> >
> >      switch (line_buf[0]) {
> >      case '!':
> > @@ -3133,6 +3396,15 @@ static void create_default_process(GDBState *s)
> >      GDBProcess *process;
> >      int max_pid = 0;
> >
> > +#if defined(CONFIG_USER_ONLY)
> > +    /*
> > +     * In QEMU user-mode emulation we want to return the real PID of the proces
> > +     * as this allows us to return proper view of /proc/$pid files as seen by
> > +     * the inferior
> > +     */
> > +    max_pid = getpid() - 1;
> > +#endif
> > +
> >      if (gdbserver_state.process_num) {
> >          max_pid = s->processes[s->process_num - 1].pid;
> >      }
> > diff --git a/linux-user/qemu.h b/linux-user/qemu.h
> > index 98dfbf2096..44a71b9740 100644
> > --- a/linux-user/qemu.h
> > +++ b/linux-user/qemu.h
> > @@ -182,6 +182,8 @@ static inline bool access_ok(CPUState *cpu, int type,
> >      return access_ok_untagged(type, cpu_untagged_addr(cpu, addr), size);
> >  }
> >
> > +int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, mode_t mode);
> > +
> >  /* NOTE __get_user and __put_user use host pointers and don't check access.
> >     These are usually used to access struct data members once the struct has
> >     been locked - usually with lock_user_struct.  */
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index b9b18a7eaf..bfc271b568 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -8233,7 +8233,7 @@ static int open_hardware(void *cpu_env, int fd)
> >  }
> >  #endif
> >
> > -static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, mode_t mode)
> > +int do_openat(void *cpu_env, int dirfd, const char *pathname, int
> > flags, mode_t mode)
>
>
> I wonder if this should be renamed to make the sense more clear?
>
> do_guest_openat? do_filtered_openat?
>

We can do, but its already in linux-user/syscall.c and I believe all
syscalls there are executed
"on behalf of guest".

> >  {
> >      struct fake_open {
> >          const char *filename;
>
> Otherwise it looks fine. Do you have time to re-spin? I would need to
> post a pull request with it by the 8th to make 7.0.
>
> --
> Alex Bennée

Thanks,
Dominik 'disconnect3d' Czarnota

Dominik Czarnota <dominik.b.czarnota@gmail.com> writes:

> Hey,
>
> I may work on this next week but I will probably not make it until the 8th :(.
>
> On Thu, 3 Mar 2022 at 13:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Disconnect3d <dominik.b.czarnota@gmail.com> writes:
>>
>> > This commit adds support for `info proc mappings` and a few other commands into
>> > the QEMU user-mode emulation gdbstub.
>> >
>> > For that, support for the following GDB remote protocol commands has been added:
>> > * vFile:setfs: pid
>> > * vFile:open: filename, flags, mode
>> > * vFile:pread: fd, count, offset
>> > * vFile:close: fd
>> > * qXfer:exec-file:read:annex:offset,length
>> >
<snip>
>> > +/*
>> > + *  Set to 1 to enable remote protocol debugging output. This output is similar
>> > + *  to the one produced by the gdbserver's --remote-debug flag with some
>> > + *  additions. Anyway, the main debug prints are:
>> > + * - getpkt ("...") which refers to received data (or, send by the GDB client)
>> > + * - putpkt ("...") which refers to sent data
>> > + */
>> > +#define ENABLE_REMOTE_DEBUG 0
>> > +
>> > +#if ENABLE_REMOTE_DEBUG
>> > +#define REMOTE_DEBUG_PRINT printf
>> > +#else
>> > +#define REMOTE_DEBUG_PRINT(...)
>> > +#endif
>>
>> We don't need this. The rest of the gdbstub has been instrumented with
>> tracepoints (try -d trace:gdbstub\* in your command line). See trace-events.
>>
>
> It would be convenient to support the gddbserver's --remote-debug flag
> but I understand it may not be wanted as it also means additional &
> unnecessary code in a production binary.
> I will remove this.

If you think the debug information will be useful by all means convert
the bits you need to tracepoints. 

>
>> > +
>> >  static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
>> >                                           uint8_t *buf, int len, bool is_write)
>> >  {
>> > @@ -554,6 +573,7 @@ static int gdb_continue_partial(char *newstates)
>> >
>> >  static void put_buffer(const uint8_t *buf, int len)
>> >  {
>> > +    REMOTE_DEBUG_PRINT("putpkt (\"%.*s\");\n", len, buf);
>> >  #ifdef CONFIG_USER_ONLY
>> >      int ret;
>> >
>> > @@ -1982,6 +2002,157 @@ static void handle_v_kill(GArray *params, void *user_ctx)
>> >      exit(0);
>> >  }
>> >
>> > +#ifdef CONFIG_USER_ONLY
>> > +/*
>> > + * Handles the `vFile:setfs: pid` command
>> > + *
>> > + * Example call: vFile:setfs:0
>> > + *
>> > + * --- From the GDB remote protocol documentation ---
>> > + * Select the filesystem on which vFile operations with filename arguments
>> > + * will operate. This is required for GDB to be able to access files on
>> > + * remote targets where the remote stub does not share a common filesystem with
>> > + * the inferior(s). If pid is nonzero, select the filesystem as seen by process
>> > + * pid. If pid is zero, select the filesystem as seen by the remote stub.
>> > + * Return 0 on success, or -1 if an error occurs. If vFile:setfs: indicates
>> > + * success, the selected filesystem remains selected until the next successful
>> > + * vFile:setfs: operation.
>> > +*/
>> > +static void handle_v_setfs(GArray *params, void *user_ctx)
>> > +{
>> > +    /*
>> > +     * We do not support different filesystem view for different pids
>> > +     * Return that all is OK, so that GDB can proceed
>> > +     */
>> > +    put_packet("F0");
>> > +}
>> > +
>> > +/*
>> > + * Handle the `vFile:open: filename, flags, mode` command
>> > + *
>> > + * We try to serve the filesystem here from the inferior point of view
>> > +
>> > + * Example call: vFile:open:6a7573742070726f62696e67,0,1c0
>> > + * (tries to open "just probing" with flags=0 mode=448)
>> > + *
>> > + * --- From the GDB remote protocol documentation ---
>> > + * Open a file at filename and return a file descriptor for it, or return
>> > + * -1 if an error occurs. The filename is a string, flags is an integer
>> > + * indicating a mask of open flags (see Open Flags), and mode is an integer
>> > + * indicating a mask of mode bits to use if the file is created
>> > + * (see mode_t Values). See open, for details of the open flags and mode
>> > + * values.
>> > + */
>> > +static void handle_v_file_open(GArray *params, void *user_ctx)
>> > +{
>> > +    uint64_t flags = get_param(params, 1)->val_ull;
>> > +    uint64_t mode = get_param(params, 2)->val_ull;
>> > +    const char *hex_filename = get_param(params, 0)->data;
>> > +
>> > +    /* Decode the filename & append a null byte so we can use it later on */
>> > +    hextomem(gdbserver_state.mem_buf, hex_filename, strlen(hex_filename));
>> > +    const char *null_byte = "\0";
>> > +    g_byte_array_append(gdbserver_state.mem_buf, (const guint8 *)null_byte, 1);
>> > +
>> > +    const char *filename = (const char *)gdbserver_state.mem_buf->data;
>> > +
>> > +    REMOTE_DEBUG_PRINT("vFile:open: filename=\"%s\" flags=%ld mode=%ld\n",
>> > +                       filename, flags, mode);
>> > +
>> > +    /*
>> > +     * On Linux we call the do_openat syscall on behalf of the inferior as it
>> > +     * handles special filepaths properly like the /proc/$pid files, which are
>> > +     * fetched by GDB for certain info (such as `info proc mappings`).
>> > +     */
>>
>> This sounds like a massive security hole to introduce without a
>> specific flag the user can explicitly enable. Semihosting has a similar
>> facility but also needs explicit configuration. Can we add a property to
>> the -gdb command line option to enable this:
>>
>>   -gdb tcp::1234,hostio=on
>>
>> ?
>
> It is, but should we really care? Both the execution and debugging of
> qemu-user emulation
> does not protect the host from arbitrary code execution and the
> proposed hostio=on flag will
> only make things less convenient and many people will just not use it
> due to not knowing that
> it exists.

Sure - but we are adding potential unexpected behaviour via the debug
stub not the guest program. The user may (although probably not) have
audited the binary they are running without realising the attack vector
the gdbstub introduces.

That said I take your point it's not super different from running
arbitrary guest code in linux-user. I think for system emulation this
functionality should definitely be an opt-in though.

Can we at least document the capability and potential security issues in
docs/system/gdb.rst?
>> > --- a/linux-user/syscall.c
>> > +++ b/linux-user/syscall.c
>> > @@ -8233,7 +8233,7 @@ static int open_hardware(void *cpu_env, int fd)
>> >  }
>> >  #endif
>> >
>> > -static int do_openat(void *cpu_env, int dirfd, const char *pathname, int flags, mode_t mode)
>> > +int do_openat(void *cpu_env, int dirfd, const char *pathname, int
>> > flags, mode_t mode)
>>
>>
>> I wonder if this should be renamed to make the sense more clear?
>>
>> do_guest_openat? do_filtered_openat?
>>
>
> We can do, but its already in linux-user/syscall.c and I believe all
> syscalls there are executed
> "on behalf of guest".

Yes - but now you are exposing it to the wider qemu code base so a more
definitive name will prevent accidental misuse elsewhere.

>
>> >  {
>> >      struct fake_open {
>> >          const char *filename;
>>
>> Otherwise it looks fine. Do you have time to re-spin? I would need to
>> post a pull request with it by the 8th to make 7.0.
>>
>> --
>> Alex Bennée
>
> Thanks,
> Dominik 'disconnect3d' Czarnota