[PATCH v2 0/3] Add additional plugin API functions to read and write memory and registers

[PATCH v2 0/3] Add additional plugin API functions to read and write memory and registers

[Rowan Hart]

This patch set follows a previous patch which added the
qemu_plugin_read_memory_vaddr function and adds a set of similar
functions to read and write registers, virtual memory, and
physical memory.

The use case I have in mind is for use of QEMU for program analysis and
testing. For example, a fuzzer which uses QEMU for emulation might wish to
inject test data into a program at runtime using qemu_plugin_write_memory_vaddr
(and likewise if testing an operating system or bare metal application using
qemu_plugin_write_memory_hwaddr). It may also wish to read the initial contents
of memory using qemu_plugin_read_memory_vaddr/hwaddr.

Similarly, a testing framework may wish to fake register values, perhaps to
simulate a device failure, perhaps by using qemu_plugin_write_register to set a
register value to an error code.

I think all this functionality works together to make QEMU
plugins more powerful and versatile, hopefully removing barriers
to using upstream QEMU for these tasks which have historically
required maintaining a QEMU fork downstream (like QEMUAFL
https://github.com/AFLplusplus/qemuafl), which is tedious, error
prone, and results in users missing out on enhancements to QEMU.

A test is provided, compile:

gcc -o tests/tcg/x86_64/inject-target tests/tcg/x86_64/inject-target.c

And run:

./build/qemu-x86_64 -d plugin --plugin build/tests/tcg/plugins/libinject.so tests/tcg/x86_64/inject-target

Hopefully after a number of tries, the inject plugin will inject the right
value into the target program, leading to a victory message. This plugin
handles simple "hypercalls", only one of which is implemented and injects
data into guest memory.

novafacing (3):
  Expose gdb_write_register function to consumers of gdbstub
  Add plugin API functions for register R/W, hwaddr R/W, vaddr W
  Add inject plugin and x86_64 target for the inject plugin

 gdbstub/gdbstub.c                |   2 +-
 include/exec/gdbstub.h           |  14 +++
 include/qemu/qemu-plugin.h       | 116 +++++++++++++++--
 plugins/api.c                    |  66 +++++++++-
 tests/tcg/plugins/inject.c       | 206 +++++++++++++++++++++++++++++++
 tests/tcg/plugins/meson.build    |   2 +-
 tests/tcg/x86_64/Makefile.target |   1 +
 tests/tcg/x86_64/inject-target.c |  27 ++++
 8 files changed, 418 insertions(+), 16 deletions(-)
 create mode 100644 tests/tcg/plugins/inject.c
 create mode 100644 tests/tcg/x86_64/inject-target.c

-- 
2.46.1

[PATCH v2 1/3] Expose gdb_write_register function to consumers of gdbstub

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

---
 gdbstub/gdbstub.c      |  2 +-
 include/exec/gdbstub.h | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index b1def7e71d..7d87a3324c 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -536,7 +536,7 @@ int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
     return 0;
 }
 
-static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
+int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
     GDBRegisterState *r;
diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index d73f424f56..584ed73fc9 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -118,6 +118,20 @@ const GDBFeature *gdb_find_static_feature(const char *xmlname);
  */
 int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
 
+/**
+ * gdb_write_register() - Write a register associated with a CPU.
+ * @cpu: The CPU associated with the register.
+ * @buf: The buffer that the register contents will be set to.
+ * @reg: The register's number returned by gdb_find_feature_register().
+ *
+ * The size of @buf must be at least the size of the register being
+ * written.
+ *
+ * Return: The number of written bytes, or 0 if an error occurred (for
+ * example, an unknown register was provided).
+ */
+int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg);
+
 /**
  * typedef GDBRegDesc - a register description from gdbstub
  */
-- 
2.46.1

[PATCH v2 2/3] Add plugin API functions for register R/W, hwaddr R/W, vaddr W

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

---
 include/qemu/qemu-plugin.h | 116 +++++++++++++++++++++++++++++++++----
 plugins/api.c              |  66 ++++++++++++++++++++-
 2 files changed, 168 insertions(+), 14 deletions(-)

diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
index 0fba36ae02..b812593e7f 100644
--- a/include/qemu/qemu-plugin.h
+++ b/include/qemu/qemu-plugin.h
@@ -65,11 +65,18 @@ typedef uint64_t qemu_plugin_id_t;
  *
  * version 4:
  * - added qemu_plugin_read_memory_vaddr
+ *
+ * version 5:
+ * - added qemu_plugin_write_memory_vaddr
+ * - added qemu_plugin_read_memory_hwaddr
+ * - added qemu_plugin_write_memory_hwaddr
+ * - added qemu_plugin_write_register
+ *
  */
 
 extern QEMU_PLUGIN_EXPORT int qemu_plugin_version;
 
-#define QEMU_PLUGIN_VERSION 4
+#define QEMU_PLUGIN_VERSION 5
 
 /**
  * struct qemu_info_t - system information for plugins
@@ -255,8 +262,6 @@ typedef struct {
  * @QEMU_PLUGIN_CB_R_REGS: callback reads the CPU's regs
  * @QEMU_PLUGIN_CB_RW_REGS: callback reads and writes the CPU's regs
  *
- * Note: currently QEMU_PLUGIN_CB_RW_REGS is unused, plugins cannot change
- * system register state.
  */
 enum qemu_plugin_cb_flags {
     QEMU_PLUGIN_CB_NO_REGS,
@@ -893,6 +898,41 @@ typedef struct {
 QEMU_PLUGIN_API
 GArray *qemu_plugin_get_registers(void);
 
+/**
+ * qemu_plugin_read_register() - read register for current vCPU
+ *
+ * @handle: a @qemu_plugin_reg_handle handle
+ * @buf: A GByteArray for the data owned by the plugin
+ *
+ * This function is only available in a context that register read access is
+ * explicitly requested via the QEMU_PLUGIN_CB_R_REGS flag.
+ *
+ * Returns the size of the read register. The content of @buf is in target byte
+ * order. On failure returns -1.
+ */
+QEMU_PLUGIN_API
+int qemu_plugin_read_register(struct qemu_plugin_register *handle,
+                              GByteArray *buf);
+
+/**
+ * qemu_plugin_write_register() - write register for current vCPU
+ *
+ * @handle: a @qemu_plugin_reg_handle handle
+ * @buf: A GByteArray for the data owned by the plugin
+ *
+ * This function is only available in a context that register write access is
+ * explicitly requested via the QEMU_PLUGIN_CB_W_REGS flag.
+ *
+ * The size of @buf must be at least the size of the requested register.
+ * Attempting to write a register with @buf smaller than the register size
+ * will result in a crash or other undesired behavior.
+ *
+ * Returns the number of bytes written. On failure returns 0.
+ */
+QEMU_PLUGIN_API
+int qemu_plugin_write_register(struct qemu_plugin_register *handle,
+                              GByteArray *buf);
+
 /**
  * qemu_plugin_read_memory_vaddr() - read from memory using a virtual address
  *
@@ -916,20 +956,72 @@ bool qemu_plugin_read_memory_vaddr(uint64_t addr,
                                    GByteArray *data, size_t len);
 
 /**
- * qemu_plugin_read_register() - read register for current vCPU
+ * qemu_plugin_write_memory_vaddr() - write to memory using a virtual address
  *
- * @handle: a @qemu_plugin_reg_handle handle
- * @buf: A GByteArray for the data owned by the plugin
+ * @addr: A virtual address to write to 
+ * @data: A byte array containing the data to write
  *
- * This function is only available in a context that register read access is
- * explicitly requested via the QEMU_PLUGIN_CB_R_REGS flag.
+ * The contents of @data will be written to memory starting at the virtual
+ * address @addr.
  *
- * Returns the size of the read register. The content of @buf is in target byte
- * order. On failure returns -1.
+ * This function does not guarantee consistency of writes, nor does it ensure
+ * that pending writes are flushed either before or after the write takes
+ * place, so callers should take care when calling this function in plugin
+ * callbacks to avoid depending on the existence of data written using this
+ * function which may be overwritten afterward.
+ *
+ * Returns true on success and false on failure.
  */
 QEMU_PLUGIN_API
-int qemu_plugin_read_register(struct qemu_plugin_register *handle,
-                              GByteArray *buf);
+bool qemu_plugin_write_memory_vaddr(uint64_t addr,
+                                   GByteArray *data);
+
+/**
+ * qemu_plugin_read_memory_vaddr() - read from memory using a hardware address
+ *
+ * @addr: A virtual address to read from
+ * @data: A byte array to store data into
+ * @len: The number of bytes to read, starting from @addr
+ *
+ * @len bytes of data is read starting at @addr and stored into @data. If @data
+ * is not large enough to hold @len bytes, it will be expanded to the necessary
+ * size, reallocating if necessary. @len must be greater than 0.
+ *
+ * This function does not ensure writes are flushed prior to reading, so
+ * callers should take care when calling this function in plugin callbacks to
+ * avoid attempting to read data which may not yet be written and should use
+ * the memory callback API instead.
+ *
+ * This function is only valid for softmmu targets.
+ *
+ * Returns true on success and false on failure.
+ */
+QEMU_PLUGIN_API
+bool qemu_plugin_read_memory_hwaddr(uint64_t addr,
+                                   GByteArray *data, size_t len);
+
+/**
+ * qemu_plugin_write_memory_vaddr() - write to memory using a hardware address
+ *
+ * @addr: A virtual address to write to 
+ * @data: A byte array containing the data to write
+ *
+ * The contents of @data will be written to memory starting at the hardware
+ * address @addr.
+ *
+ * This function does not guarantee consistency of writes, nor does it ensure
+ * that pending writes are flushed either before or after the write takes
+ * place, so callers should take care when calling this function in plugin
+ * callbacks to avoid depending on the existence of data written using this
+ * function which may be overwritten afterward.
+ *
+ * This function is only valid for softmmu targets.
+ *
+ * Returns true on success and false on failure.
+ */
+QEMU_PLUGIN_API
+bool qemu_plugin_write_memory_hwaddr(uint64_t addr,
+                                   GByteArray *data);
 
 /**
  * qemu_plugin_scoreboard_new() - alloc a new scoreboard
diff --git a/plugins/api.c b/plugins/api.c
index 24ea64e2de..4a84cf4dfe 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -560,6 +560,24 @@ GArray *qemu_plugin_get_registers(void)
     return create_register_handles(regs);
 }
 
+int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
+{
+    g_assert(current_cpu);
+
+    return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
+}
+
+int qemu_plugin_write_register(struct qemu_plugin_register *reg, GByteArray *buf)
+{
+    g_assert(current_cpu);
+
+    if (buf->len == 0) {
+        return 0;
+    }
+
+    return gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) - 1);
+}
+
 bool qemu_plugin_read_memory_vaddr(vaddr addr, GByteArray *data, size_t len)
 {
     g_assert(current_cpu);
@@ -580,13 +598,57 @@ bool qemu_plugin_read_memory_vaddr(vaddr addr, GByteArray *data, size_t len)
     return true;
 }
 
-int qemu_plugin_read_register(struct qemu_plugin_register *reg, GByteArray *buf)
+bool qemu_plugin_write_memory_vaddr(vaddr addr, GByteArray *data)
 {
     g_assert(current_cpu);
 
-    return gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) - 1);
+    if (data->len == 0) {
+        return false;
+    }
+
+    int result = cpu_memory_rw_debug(current_cpu, addr, data->data,
+                                     data->len, true);
+
+    if (result < 0) {
+        return false;
+    }
+
+    return true;
+}
+
+bool qemu_plugin_read_memory_hwaddr(hwaddr addr, GByteArray *data, size_t len)
+{
+#ifdef CONFIG_SOFTMMU
+    if (len == 0) {
+        return false;
+    }
+
+    g_byte_array_set_size(data, len);
+
+    cpu_physical_memory_rw(addr, data->data, data->len, false);
+
+    return true;
+#else
+    return false;
+#endif
 }
 
+bool qemu_plugin_write_memory_hwaddr(hwaddr addr, GByteArray *data)
+{
+#ifdef CONFIG_SOFTMMU
+    if (data->len == 0) {
+        return false;
+    }
+
+    cpu_physical_memory_rw(addr, data->data, data->len, true);
+
+    return true;
+#else
+    return false;
+#endif
+}
+
+
 struct qemu_plugin_scoreboard *qemu_plugin_scoreboard_new(size_t element_size)
 {
     return plugin_scoreboard_new(element_size);
-- 
2.46.1

[PATCH v2 3/3] Add inject plugin and x86_64 target for the inject plugin

[Rowan Hart]

From: novafacing <rowanbhart@gmail.com>

---
 tests/tcg/plugins/inject.c       | 206 +++++++++++++++++++++++++++++++
 tests/tcg/plugins/meson.build    |   2 +-
 tests/tcg/x86_64/Makefile.target |   1 +
 tests/tcg/x86_64/inject-target.c |  27 ++++
 4 files changed, 235 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/plugins/inject.c
 create mode 100644 tests/tcg/x86_64/inject-target.c

diff --git a/tests/tcg/plugins/inject.c b/tests/tcg/plugins/inject.c
new file mode 100644
index 0000000000..9edc2cd34e
--- /dev/null
+++ b/tests/tcg/plugins/inject.c
@@ -0,0 +1,206 @@
+/*
+ * Copyright (C) 2024, Rowan Hart <rowanbhart@gmail.com>
+ *
+ * License: GNU GPL, version 2 or later.
+ *   See the COPYING file in the top-level directory.
+ */
+#include "glib.h"
+#include <assert.h>
+#include <inttypes.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <qemu-plugin.h>
+
+/*
+ * Specifies a Hypercall for an architecture:
+ *
+ * - Architecture name
+ * - Whether it is enabled
+ * - The hypercall instruction
+ * - The register names to pass the hypercall # and args
+ */
+struct HypercallSpec {
+    const char *name;
+    const bool enabled;
+    const char *hypercall;
+    const bool little_endian;
+    const char *num_reg;
+    const char *arg0_reg;
+    const char *arg1_reg;
+};
+
+static const struct HypercallSpec *hypercall_spec;
+
+static const struct HypercallSpec hypercall_specs[] = {
+    { "aarch64", false, NULL, true, 0, 0, 0 },
+    { "aarch64_be", false, NULL, false, 0, 0, 0 },
+    { "alpha", false, NULL, true, 0, 0, 0 },
+    { "arm", false, NULL, true, 0, 0, 0 },
+    { "armeb", false, NULL, false, 0, 0, 0 },
+    { "avr", false, NULL, true, 0, 0, 0 },
+    { "hexagon", false, NULL, true, 0, 0, 0 },
+    { "hppa", false, NULL, false, 0, 0, 0 },
+    { "i386", false, NULL, true, 0, 0, 0 },
+    { "loongarch64", false, NULL, true, 0, 0, 0 },
+    { "m68k", false, NULL, false, 0, 0, 0 },
+    { "microblaze", false, NULL, false, 0, 0, 0 },
+    { "microblazeel", false, NULL, true, 0, 0, 0 },
+    { "mips", false, NULL, false, 0, 0, 0 },
+    { "mips64", false, NULL, false, 0, 0, 0 },
+    { "mips64el", false, NULL, true, 0, 0, 0 },
+    { "mipsel", false, NULL, true, 0, 0, 0 },
+    { "mipsn32", false, NULL, false, 0, 0, 0 },
+    { "mipsn32el", false, NULL, true, 0, 0, 0 },
+    { "or1k", false, NULL, false, 0, 0, 0 },
+    { "ppc", false, NULL, false, 0, 0, 0 },
+    { "ppc64", false, NULL, false, 0, 0, 0 },
+    { "ppc64le", false, NULL, true, 0, 0, 0 },
+    { "riscv32", false, NULL, true, 0, 0, 0 },
+    { "riscv64", false, NULL, true, 0, 0, 0 },
+    { "rx", false, NULL, true, 0, 0, 0 },
+    { "s390x", false, NULL, false, 0, 0, 0 },
+    { "sh4", false, NULL, true, 0, 0, 0 },
+    { "sh4eb", false, NULL, false, 0, 0, 0 },
+    { "sparc", false, NULL, false, 0, 0, 0 },
+    { "sparc32plus", false, NULL, false, 0, 0, 0 },
+    { "sparc64", false, NULL, false, 0, 0, 0 },
+    { "tricore", false, NULL, true, 0, 0, 0 },
+    { "x86_64", true, "\x0f\xa2", true, "rax", "rdi", "rsi" },
+    { "xtensa", false, NULL, true, 0, 0, 0 },
+    { "xtensaeb", false, NULL, false, 0, 0, 0 },
+    { NULL, false, NULL, false, 0, 0, 0 },
+};
+
+QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
+
+/*
+ * Returns a handle to a register with a given name, or NULL if there is no
+ * such register.
+ */
+static struct qemu_plugin_register *get_register(const char *name)
+{
+    GArray *registers = qemu_plugin_get_registers();
+
+    struct qemu_plugin_register *handle = NULL;
+
+    qemu_plugin_reg_descriptor *reg_descriptors =
+        (qemu_plugin_reg_descriptor *)registers->data;
+
+    for (size_t i = 0; i < registers->len; i++) {
+        if (!strcmp(reg_descriptors[i].name, name)) {
+            handle = reg_descriptors[i].handle;
+        }
+    }
+
+    g_array_free(registers, true);
+
+    return handle;
+}
+
+/*
+ * Transforms a byte array with at most 8 entries into a uint64_t
+ * depending on the target machine's endianness.
+ */
+static uint64_t byte_array_to_uint64(GByteArray *buf)
+{
+    uint64_t value = 0;
+    if (hypercall_spec->little_endian) {
+        for (int i = 0; i < buf->len && i < sizeof(uint64_t); i++) {
+            value |= ((uint64_t)buf->data[i]) << (i * 8);
+        }
+    } else {
+        for (int i = 0; i < buf->len && i < sizeof(uint64_t); i++) {
+            value |= ((uint64_t)buf->data[i]) << ((buf->len - 1 - i) * 8);
+        }
+    }
+    return value;
+}
+
+/*
+ * Handle a "hyperacll" instruction, which has some special meaning for this
+ * plugin.
+ */
+static void hypercall(unsigned int vcpu_index, void *userdata)
+{
+    uint64_t num = 0, arg0 = 0, arg1 = 0;
+    GByteArray *buf = g_byte_array_new();
+    qemu_plugin_read_register(get_register(hypercall_spec->num_reg), buf);
+    num = byte_array_to_uint64(buf);
+
+    g_byte_array_set_size(buf, 0);
+    qemu_plugin_read_register(get_register(hypercall_spec->arg0_reg), buf);
+    arg0 = byte_array_to_uint64(buf);
+
+    g_byte_array_set_size(buf, 0);
+    qemu_plugin_read_register(get_register(hypercall_spec->arg1_reg), buf);
+    arg1 = byte_array_to_uint64(buf);
+
+    switch (num) {
+    /*
+     * The write hypercall (#0x13371337) tells the plugin to write random bytes
+     * of a given size into the memory of the emulated system at a particular
+     * vaddr
+     */
+    case 0x13371337: {
+        GByteArray *data = g_byte_array_new();
+        g_byte_array_set_size(data, arg1);
+        for (uint64_t i = 0; i < arg1; i++) {
+            data->data[i] = (uint8_t)g_random_int();
+        }
+        qemu_plugin_write_memory_vaddr(arg0, data);
+        break;
+    }
+    default:
+        break;
+    }
+
+    g_byte_array_free(buf, TRUE);
+}
+
+/*
+ * Callback on translation of a translation block.
+ */
+static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
+{
+    for (size_t i = 0; i < qemu_plugin_tb_n_insns(tb); i++) {
+        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
+        GByteArray *insn_data = g_byte_array_new();
+        size_t insn_len = qemu_plugin_insn_size(insn);
+        g_byte_array_set_size(insn_data, insn_len);
+        qemu_plugin_insn_data(insn, insn_data->data, insn_data->len);
+        if (!memcmp(insn_data->data, hypercall_spec->hypercall, insn_data->len)) {
+            qemu_plugin_register_vcpu_insn_exec_cb(insn, hypercall,
+                                                   QEMU_PLUGIN_CB_R_REGS, NULL);
+        }
+        g_byte_array_free(insn_data, true);
+    }
+}
+
+
+/*
+ * Called when the plugin is installed
+ */
+QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
+                                           const qemu_info_t *info, int argc,
+                                           char **argv)
+{
+    hypercall_spec = &hypercall_specs[0];
+    while (hypercall_spec->name != NULL) {
+        if (!strcmp(hypercall_spec->name, info->target_name)) {
+            break;
+        }
+        hypercall_spec++;
+    }
+
+    if (hypercall_spec->name == NULL) {
+        qemu_plugin_outs("Error: no hypercall spec.");
+        return -1;
+    }
+
+    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
+
+    return 0;
+}
diff --git a/tests/tcg/plugins/meson.build b/tests/tcg/plugins/meson.build
index f847849b1b..96782416d3 100644
--- a/tests/tcg/plugins/meson.build
+++ b/tests/tcg/plugins/meson.build
@@ -1,6 +1,6 @@
 t = []
 if get_option('plugins')
-  foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'syscall']
+  foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'syscall', 'inject']
     if host_os == 'windows'
       t += shared_module(i, files(i + '.c') + '../../../contrib/plugins/win32_linker.c',
                         include_directories: '../../../include/qemu',
diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
index d6dff559c7..7c8e21636d 100644
--- a/tests/tcg/x86_64/Makefile.target
+++ b/tests/tcg/x86_64/Makefile.target
@@ -18,6 +18,7 @@ X86_64_TESTS += adox
 X86_64_TESTS += test-1648
 X86_64_TESTS += test-2175
 X86_64_TESTS += cross-modifying-code
+X86_64_TESTS += inject-target
 TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
 else
 TESTS=$(MULTIARCH_TESTS)
diff --git a/tests/tcg/x86_64/inject-target.c b/tests/tcg/x86_64/inject-target.c
new file mode 100644
index 0000000000..c886e5ab8b
--- /dev/null
+++ b/tests/tcg/x86_64/inject-target.c
@@ -0,0 +1,27 @@
+#include <stddef.h>
+#include <stdint.h>
+#include <stdio.h>
+
+#define hypercall(num, arg0, arg1)                                \
+    unsigned int _a __attribute__((unused)) = 0;                  \
+    unsigned int _b __attribute__((unused)) = 0;                  \
+    unsigned int _c __attribute__((unused)) = 0;                  \
+    unsigned int _d __attribute__((unused)) = 0;                  \
+    __asm__ __volatile__("cpuid\n\t"                              \
+                         : "=a"(_a), "=b"(_b), "=c"(_c), "=d"(_d) \
+                         : "a"(num), "D"(arg0), "S"(arg1));
+
+int main(void)
+{
+    uint16_t value;
+
+    for (size_t i = 0; i < 1000000; i++) {
+        hypercall(0x13371337, &value, sizeof(value));
+        if (value == 0x1337) {
+            printf("Victory!\n");
+            return 0;
+        }
+    }
+    return 1;
+}
+
-- 
2.46.1

Re: [PATCH v2 0/3] Add additional plugin API functions to read and write memory and registers

[Pierrick Bouvier]

Hi Rowan,

thanks for this submission!

On 12/6/24 02:26, Rowan Hart wrote:
> This patch set follows a previous patch which added the
> qemu_plugin_read_memory_vaddr function and adds a set of similar
> functions to read and write registers, virtual memory, and
> physical memory.
> 
> The use case I have in mind is for use of QEMU for program analysis and
> testing. For example, a fuzzer which uses QEMU for emulation might wish to
> inject test data into a program at runtime using qemu_plugin_write_memory_vaddr
> (and likewise if testing an operating system or bare metal application using
> qemu_plugin_write_memory_hwaddr). It may also wish to read the initial contents
> of memory using qemu_plugin_read_memory_vaddr/hwaddr.
> 

I am personally in favor to adding such features in upstream QEMU, but 
we should discuss it with the maintainers, because it would allow to 
change the state of execution, which is something qemu plugins actively 
didn't try to do. It's a real paradigm shift for plugins.

By writing to memory/registers, we can start replacing instructions and 
control flow, and there is a whole set of consequences to that.

> Similarly, a testing framework may wish to fake register values, perhaps to
> simulate a device failure, perhaps by using qemu_plugin_write_register to set a
> register value to an error code.
> 
> I think all this functionality works together to make QEMU
> plugins more powerful and versatile, hopefully removing barriers
> to using upstream QEMU for these tasks which have historically
> required maintaining a QEMU fork downstream (like QEMUAFL
> https://github.com/AFLplusplus/qemuafl), which is tedious, error
> prone, and results in users missing out on enhancements to QEMU.
> 
> A test is provided, compile:
> 
> gcc -o tests/tcg/x86_64/inject-target tests/tcg/x86_64/inject-target.c
> 
> And run:
> 
> ./build/qemu-x86_64 -d plugin --plugin build/tests/tcg/plugins/libinject.so tests/tcg/x86_64/inject-target
> 
> Hopefully after a number of tries, the inject plugin will inject the right
> value into the target program, leading to a victory message. This plugin
> handles simple "hypercalls", only one of which is implemented and injects
> data into guest memory.
> 

The hypercall functionality would be useful for plugins as a whole. And 
I think it definitely deserves to be worked on, if maintainers are open 
to that as well.

> novafacing (3):
>    Expose gdb_write_register function to consumers of gdbstub
>    Add plugin API functions for register R/W, hwaddr R/W, vaddr W
>    Add inject plugin and x86_64 target for the inject plugin
> 
>   gdbstub/gdbstub.c                |   2 +-
>   include/exec/gdbstub.h           |  14 +++
>   include/qemu/qemu-plugin.h       | 116 +++++++++++++++--
>   plugins/api.c                    |  66 +++++++++-
>   tests/tcg/plugins/inject.c       | 206 +++++++++++++++++++++++++++++++
>   tests/tcg/plugins/meson.build    |   2 +-
>   tests/tcg/x86_64/Makefile.target |   1 +
>   tests/tcg/x86_64/inject-target.c |  27 ++++
>   8 files changed, 418 insertions(+), 16 deletions(-)
>   create mode 100644 tests/tcg/plugins/inject.c
>   create mode 100644 tests/tcg/x86_64/inject-target.c
> 

Regards,
Pierrick

Re: [PATCH v2 3/3] Add inject plugin and x86_64 target for the inject plugin

[Pierrick Bouvier]

On 12/6/24 02:26, Rowan Hart wrote:
> From: novafacing <rowanbhart@gmail.com>
> 
> ---
>   tests/tcg/plugins/inject.c       | 206 +++++++++++++++++++++++++++++++
>   tests/tcg/plugins/meson.build    |   2 +-
>   tests/tcg/x86_64/Makefile.target |   1 +
>   tests/tcg/x86_64/inject-target.c |  27 ++++
>   4 files changed, 235 insertions(+), 1 deletion(-)
>   create mode 100644 tests/tcg/plugins/inject.c
>   create mode 100644 tests/tcg/x86_64/inject-target.c
> 
> diff --git a/tests/tcg/plugins/inject.c b/tests/tcg/plugins/inject.c
> new file mode 100644
> index 0000000000..9edc2cd34e
> --- /dev/null
> +++ b/tests/tcg/plugins/inject.c

Could we find a better name?

> @@ -0,0 +1,206 @@
> +/*
> + * Copyright (C) 2024, Rowan Hart <rowanbhart@gmail.com>
> + *
> + * License: GNU GPL, version 2 or later.
> + *   See the COPYING file in the top-level directory.
> + */

We can add a comment here about what the plugin is doing.

> +#include "glib.h"
> +#include <assert.h>
> +#include <inttypes.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <qemu-plugin.h>
> +
> +/*
> + * Specifies a Hypercall for an architecture:
> + *
> + * - Architecture name
> + * - Whether it is enabled
> + * - The hypercall instruction
> + * - The register names to pass the hypercall # and args
> + */
> +struct HypercallSpec {
> +    const char *name;
> +    const bool enabled;
> +    const char *hypercall;
> +    const bool little_endian;
> +    const char *num_reg;
> +    const char *arg0_reg;
> +    const char *arg1_reg;
> +};
> +
> +static const struct HypercallSpec *hypercall_spec;
> +
> +static const struct HypercallSpec hypercall_specs[] = {
> +    { "aarch64", false, NULL, true, 0, 0, 0 },
> +    { "aarch64_be", false, NULL, false, 0, 0, 0 },
> +    { "alpha", false, NULL, true, 0, 0, 0 },
> +    { "arm", false, NULL, true, 0, 0, 0 },
> +    { "armeb", false, NULL, false, 0, 0, 0 },
> +    { "avr", false, NULL, true, 0, 0, 0 },
> +    { "hexagon", false, NULL, true, 0, 0, 0 },
> +    { "hppa", false, NULL, false, 0, 0, 0 },
> +    { "i386", false, NULL, true, 0, 0, 0 },
> +    { "loongarch64", false, NULL, true, 0, 0, 0 },
> +    { "m68k", false, NULL, false, 0, 0, 0 },
> +    { "microblaze", false, NULL, false, 0, 0, 0 },
> +    { "microblazeel", false, NULL, true, 0, 0, 0 },
> +    { "mips", false, NULL, false, 0, 0, 0 },
> +    { "mips64", false, NULL, false, 0, 0, 0 },
> +    { "mips64el", false, NULL, true, 0, 0, 0 },
> +    { "mipsel", false, NULL, true, 0, 0, 0 },
> +    { "mipsn32", false, NULL, false, 0, 0, 0 },
> +    { "mipsn32el", false, NULL, true, 0, 0, 0 },
> +    { "or1k", false, NULL, false, 0, 0, 0 },
> +    { "ppc", false, NULL, false, 0, 0, 0 },
> +    { "ppc64", false, NULL, false, 0, 0, 0 },
> +    { "ppc64le", false, NULL, true, 0, 0, 0 },
> +    { "riscv32", false, NULL, true, 0, 0, 0 },
> +    { "riscv64", false, NULL, true, 0, 0, 0 },
> +    { "rx", false, NULL, true, 0, 0, 0 },
> +    { "s390x", false, NULL, false, 0, 0, 0 },
> +    { "sh4", false, NULL, true, 0, 0, 0 },
> +    { "sh4eb", false, NULL, false, 0, 0, 0 },
> +    { "sparc", false, NULL, false, 0, 0, 0 },
> +    { "sparc32plus", false, NULL, false, 0, 0, 0 },
> +    { "sparc64", false, NULL, false, 0, 0, 0 },
> +    { "tricore", false, NULL, true, 0, 0, 0 },
> +    { "x86_64", true, "\x0f\xa2", true, "rax", "rdi", "rsi" },
> +    { "xtensa", false, NULL, true, 0, 0, 0 },
> +    { "xtensaeb", false, NULL, false, 0, 0, 0 },
> +    { NULL, false, NULL, false, 0, 0, 0 },
> +};
> +
> +QEMU_PLUGIN_EXPORT int qemu_plugin_version = QEMU_PLUGIN_VERSION;
> +
> +/*
> + * Returns a handle to a register with a given name, or NULL if there is no
> + * such register.
> + */
> +static struct qemu_plugin_register *get_register(const char *name)
> +{
> +    GArray *registers = qemu_plugin_get_registers();
> +
> +    struct qemu_plugin_register *handle = NULL;
> +
> +    qemu_plugin_reg_descriptor *reg_descriptors =
> +        (qemu_plugin_reg_descriptor *)registers->data;
> +
> +    for (size_t i = 0; i < registers->len; i++) {
> +        if (!strcmp(reg_descriptors[i].name, name)) {
> +            handle = reg_descriptors[i].handle;
> +        }
> +    }
> +
> +    g_array_free(registers, true);
> +
> +    return handle;
> +}
> +
> +/*
> + * Transforms a byte array with at most 8 entries into a uint64_t
> + * depending on the target machine's endianness.
> + */
> +static uint64_t byte_array_to_uint64(GByteArray *buf)
> +{
> +    uint64_t value = 0;
> +    if (hypercall_spec->little_endian) {
> +        for (int i = 0; i < buf->len && i < sizeof(uint64_t); i++) {
> +            value |= ((uint64_t)buf->data[i]) << (i * 8);
> +        }
> +    } else {
> +        for (int i = 0; i < buf->len && i < sizeof(uint64_t); i++) {
> +            value |= ((uint64_t)buf->data[i]) << ((buf->len - 1 - i) * 8);
> +        }
> +    }
> +    return value;
> +}
> +
> +/*
> + * Handle a "hyperacll" instruction, which has some special meaning for this
> + * plugin.
> + */
> +static void hypercall(unsigned int vcpu_index, void *userdata)
> +{
> +    uint64_t num = 0, arg0 = 0, arg1 = 0;
> +    GByteArray *buf = g_byte_array_new();
> +    qemu_plugin_read_register(get_register(hypercall_spec->num_reg), buf);
> +    num = byte_array_to_uint64(buf);
> +
> +    g_byte_array_set_size(buf, 0);
> +    qemu_plugin_read_register(get_register(hypercall_spec->arg0_reg), buf);
> +    arg0 = byte_array_to_uint64(buf);
> +
> +    g_byte_array_set_size(buf, 0);
> +    qemu_plugin_read_register(get_register(hypercall_spec->arg1_reg), buf);
> +    arg1 = byte_array_to_uint64(buf);
> +
> +    switch (num) {
> +    /*
> +     * The write hypercall (#0x13371337) tells the plugin to write random bytes
> +     * of a given size into the memory of the emulated system at a particular
> +     * vaddr
> +     */

One challenge with picking a random value, is how to ensure this pattern 
has no other meaning for all architectures? I'm not sure we can find a 
single pattern of bytes that works for all arch, even though that would 
be definitely stylish :).

In more, it seems that we are reinventing the syscall interface, while 
we already have it. But as the current instrumentation only works for 
user-mode, having a specific hypercall interface might be worth it for 
plugins, so system mode could benefit from it too.

The work done here could serve later to define a proper interface.

> +    case 0x13371337: {
> +        GByteArray *data = g_byte_array_new();
> +        g_byte_array_set_size(data, arg1);
> +        for (uint64_t i = 0; i < arg1; i++) {
> +            data->data[i] = (uint8_t)g_random_int();
> +        }
> +        qemu_plugin_write_memory_vaddr(arg0, data);
> +        break;
> +    }
> +    default:
> +        break;
> +    }
> +
> +    g_byte_array_free(buf, TRUE);
> +}
> +
> +/*
> + * Callback on translation of a translation block.
> + */
> +static void vcpu_tb_trans(qemu_plugin_id_t id, struct qemu_plugin_tb *tb)
> +{
> +    for (size_t i = 0; i < qemu_plugin_tb_n_insns(tb); i++) {
> +        struct qemu_plugin_insn *insn = qemu_plugin_tb_get_insn(tb, i);
> +        GByteArray *insn_data = g_byte_array_new();
> +        size_t insn_len = qemu_plugin_insn_size(insn);
> +        g_byte_array_set_size(insn_data, insn_len);
> +        qemu_plugin_insn_data(insn, insn_data->data, insn_data->len);
> +        if (!memcmp(insn_data->data, hypercall_spec->hypercall, insn_data->len)) {
> +            qemu_plugin_register_vcpu_insn_exec_cb(insn, hypercall,
> +                                                   QEMU_PLUGIN_CB_R_REGS, NULL);
> +        }
> +        g_byte_array_free(insn_data, true);
> +    }
> +}
> +
> +
> +/*
> + * Called when the plugin is installed
> + */
> +QEMU_PLUGIN_EXPORT int qemu_plugin_install(qemu_plugin_id_t id,
> +                                           const qemu_info_t *info, int argc,
> +                                           char **argv)
> +{
> +    hypercall_spec = &hypercall_specs[0];
> +    while (hypercall_spec->name != NULL) {
> +        if (!strcmp(hypercall_spec->name, info->target_name)) {
> +            break;
> +        }
> +        hypercall_spec++;
> +    }
> +
> +    if (hypercall_spec->name == NULL) {
> +        qemu_plugin_outs("Error: no hypercall spec.");
> +        return -1;
> +    }
> +
> +    qemu_plugin_register_vcpu_tb_trans_cb(id, vcpu_tb_trans);
> +
> +    return 0;
> +}
> diff --git a/tests/tcg/plugins/meson.build b/tests/tcg/plugins/meson.build
> index f847849b1b..96782416d3 100644
> --- a/tests/tcg/plugins/meson.build
> +++ b/tests/tcg/plugins/meson.build
> @@ -1,6 +1,6 @@
>   t = []
>   if get_option('plugins')
> -  foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'syscall']
> +  foreach i : ['bb', 'empty', 'inline', 'insn', 'mem', 'syscall', 'inject']
>       if host_os == 'windows'
>         t += shared_module(i, files(i + '.c') + '../../../contrib/plugins/win32_linker.c',
>                           include_directories: '../../../include/qemu',
> diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
> index d6dff559c7..7c8e21636d 100644
> --- a/tests/tcg/x86_64/Makefile.target
> +++ b/tests/tcg/x86_64/Makefile.target
> @@ -18,6 +18,7 @@ X86_64_TESTS += adox
>   X86_64_TESTS += test-1648
>   X86_64_TESTS += test-2175
>   X86_64_TESTS += cross-modifying-code
> +X86_64_TESTS += inject-target
>   TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
>   else
>   TESTS=$(MULTIARCH_TESTS)
> diff --git a/tests/tcg/x86_64/inject-target.c b/tests/tcg/x86_64/inject-target.c
> new file mode 100644
> index 0000000000..c886e5ab8b
> --- /dev/null
> +++ b/tests/tcg/x86_64/inject-target.c
> @@ -0,0 +1,27 @@
> +#include <stddef.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +
> +#define hypercall(num, arg0, arg1)                                \
> +    unsigned int _a __attribute__((unused)) = 0;                  \
> +    unsigned int _b __attribute__((unused)) = 0;                  \
> +    unsigned int _c __attribute__((unused)) = 0;                  \
> +    unsigned int _d __attribute__((unused)) = 0;                  \
> +    __asm__ __volatile__("cpuid\n\t"                              \
> +                         : "=a"(_a), "=b"(_b), "=c"(_c), "=d"(_d) \
> +                         : "a"(num), "D"(arg0), "S"(arg1));
> +
> +int main(void)
> +{
> +    uint16_t value;
> +
> +    for (size_t i = 0; i < 1000000; i++) {
> +        hypercall(0x13371337, &value, sizeof(value));
> +        if (value == 0x1337) {
> +            printf("Victory!\n");
> +            return 0;
> +        }
> +    }
> +    return 1;
> +}
> +

Re: [PATCH v2 0/3] Add additional plugin API functions to read and write memory and registers

[Rowan Hart]

> I am personally in favor to adding such features in upstream QEMU, but we should discuss it with the maintainers, because it would allow to change the state of execution, which is something qemu plugins actively didn't try to do. It's a real paradigm shift for plugins.
> 
> By writing to memory/registers, we can start replacing instructions and control flow, and there is a whole set of consequences to that.
> 

Totally agree! As much as I really want this functionality for plugins, I think
alignment on it is quite important. I can see very cool use cases for being
able to replace instructions and control flow to allow hooking functions,
hotpatching, and so forth.

I don't really know the edge cases here so your expertise will be helpful. In
the worst case I can imagine: what would happen if a callback overwrote the
next insn? I'm not sure what behavior I would expect if that insn has already
been translated as part of the same tb. That said, the plugin is aware of which
insns have already been translated, so maybe it is not unreasonable to just
document this as a "don't do that". Let me know what you think.

> The hypercall functionality would be useful for plugins as a whole. And I think it definitely deserves to be worked on, if maintainers are open to that as well.

Sure, I'd be happy to work on this some more. At least on the fuzzing side of
things, the way hypercalls are done across hypervisors (QEMU, Bochs, etc) is
pretty consistent so I think we could provide a useful common set of
functionality. The reason I did the bare minimum here is I'm not familiar with
every architecture, and a good NOP needs to be chosen for each one along with a
reasonable way to pass some arguments -- I don't know if I'm the right person
to make that call.

Glad to hear you're for this idea!

-Rowan

Re: [PATCH v2 3/3] Add inject plugin and x86_64 target for the inject plugin

[Rowan Hart]

>> +++ b/tests/tcg/plugins/inject.c
> 
> Could we find a better name? 

For sure, maybe "hypercalls.c" since that's really what it's mostly about.

>> @@ -0,0 +1,206 @@
>> +/*
>> + * Copyright (C) 2024, Rowan Hart <rowanbhart@gmail.com>
>> + *
>> + * License: GNU GPL, version 2 or later.
>> + *   See the COPYING file in the top-level directory.
>> + */
> 
> We can add a comment here about what the plugin is doing. 

Will do!

> One challenge with picking a random value, is how to ensure this pattern has no other meaning for all architectures? I'm not sure we can find a single pattern of bytes that works for all arch, even though that would be definitely stylish :).
> 
> In more, it seems that we are reinventing the syscall interface, while we already have it. But as the current instrumentation only works for user-mode, having a specific hypercall interface might be worth it for plugins, so system mode could benefit from it too.
> 
> The work done here could serve later to define a proper interface. 


I'll see what I can do about this. SIMICS supports many architectures and has a
"magic instruction" interface[0] (basically hypercalls) and has these
instructions defined per-architecture in a way that at minimum there are 12
values available which work on every architecture the simulator supports. QEMU
supports more architectures than SIMICS but I think we could start there and
follow a similar approach.

[0]:
https://intel.github.io/tsffs/simics/simics-user-guide/breakpoints.html#Magic-Breakpoints

-Rowan

Re: [PATCH v2 3/3] Add inject plugin and x86_64 target for the inject plugin

[Pierrick Bouvier]

On 12/6/24 17:02, Rowan Hart wrote:
>>> +++ b/tests/tcg/plugins/inject.c
>>
>> Could we find a better name?
> 
> For sure, maybe "hypercalls.c" since that's really what it's mostly about.
> 

Sounds good.

>>> @@ -0,0 +1,206 @@
>>> +/*
>>> + * Copyright (C) 2024, Rowan Hart <rowanbhart@gmail.com>
>>> + *
>>> + * License: GNU GPL, version 2 or later.
>>> + *   See the COPYING file in the top-level directory.
>>> + */
>>
>> We can add a comment here about what the plugin is doing.
> 
> Will do!
> 
>> One challenge with picking a random value, is how to ensure this pattern has no other meaning for all architectures? I'm not sure we can find a single pattern of bytes that works for all arch, even though that would be definitely stylish :).
>>
>> In more, it seems that we are reinventing the syscall interface, while we already have it. But as the current instrumentation only works for user-mode, having a specific hypercall interface might be worth it for plugins, so system mode could benefit from it too.
>>
>> The work done here could serve later to define a proper interface.
> 
> 
> I'll see what I can do about this. SIMICS supports many architectures and has a
> "magic instruction" interface[0] (basically hypercalls) and has these
> instructions defined per-architecture in a way that at minimum there are 12
> values available which work on every architecture the simulator supports. QEMU
> supports more architectures than SIMICS but I think we could start there and
> follow a similar approach.
> 
> [0]:
> https://intel.github.io/tsffs/simics/simics-user-guide/breakpoints.html#Magic-Breakpoints
> 

Looks like a good model to reuse if we want to implement something similar.

> -Rowan

Re: [PATCH v2 0/3] Add additional plugin API functions to read and write memory and registers

[Pierrick Bouvier]

On 12/6/24 16:57, Rowan Hart wrote:
>> I am personally in favor to adding such features in upstream QEMU, but we should discuss it with the maintainers, because it would allow to change the state of execution, which is something qemu plugins actively didn't try to do. It's a real paradigm shift for plugins.
>>
>> By writing to memory/registers, we can start replacing instructions and control flow, and there is a whole set of consequences to that.
>>
> 
> Totally agree! As much as I really want this functionality for plugins, I think
> alignment on it is quite important. I can see very cool use cases for being
> able to replace instructions and control flow to allow hooking functions,
> hotpatching, and so forth.
> 
> I don't really know the edge cases here so your expertise will be helpful. In
> the worst case I can imagine: what would happen if a callback overwrote the
> next insn? I'm not sure what behavior I would expect if that insn has already
> been translated as part of the same tb. That said, the plugin is aware of which
> insns have already been translated, so maybe it is not unreasonable to just
> document this as a "don't do that". Let me know what you think.
> 

In the end, if we implement something to modify running code, we should 
make sure it works as expected (flushing the related tb). I am not sure 
about the current status, and all the changes that would be needed, but 
it's something we should discuss before implementing.

More globally, let's wait to hear feedback from maintainers to see if 
they are open to the idea or not. A "hard" no would end it there.

>> The hypercall functionality would be useful for plugins as a whole. And I think it definitely deserves to be worked on, if maintainers are open to that as well.
> 
> Sure, I'd be happy to work on this some more. At least on the fuzzing side of
> things, the way hypercalls are done across hypervisors (QEMU, Bochs, etc) is
> pretty consistent so I think we could provide a useful common set of
> functionality. The reason I did the bare minimum here is I'm not familiar with
> every architecture, and a good NOP needs to be chosen for each one along with a
> reasonable way to pass some arguments -- I don't know if I'm the right person
> to make that call.
> 

We have been discussing something like that for system mode recently, so 
it would definitely be useful.

IMHO, it's open for anyone to contribute this, the plugins area is not a 
private garden where only chosen ones can change things. Just be 
prepared for change requests, and multiple versions before the final one.

Same on this one, we'll see if maintainers are ok with the idea.

> Glad to hear you're for this idea!
> 
> -Rowan

Thanks,
Pierrick

Re: [PATCH v2 0/3] Add additional plugin API functions to read and write memory and registers

[Alex Bennée]

Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:

> On 12/6/24 16:57, Rowan Hart wrote:
>>> I am personally in favor to adding such features in upstream QEMU,
>>> but we should discuss it with the maintainers, because it would
>>> allow to change the state of execution, which is something qemu
>>> plugins actively didn't try to do. It's a real paradigm shift for
>>> plugins.
>>>
>>> By writing to memory/registers, we can start replacing instructions and control flow, and there is a whole set of consequences to that.
>>>
>> Totally agree! As much as I really want this functionality for
>> plugins, I think
>> alignment on it is quite important. I can see very cool use cases for being
>> able to replace instructions and control flow to allow hooking functions,
>> hotpatching, and so forth.

I think currently that makes quite a lot of demands on the translator to
make sure things are kept consistent.

We have been talking about maybe enabling hypercalls of some sort to
allow for hooking explicit function boundaries in linux-user. A natural
extension of that would be for host library bypass functions. I'm unsure
of how that would apply in system emulation mode though where things are
handled on a lot more granular level.

>> I don't really know the edge cases here so your expertise will be
>> helpful. In
>> the worst case I can imagine: what would happen if a callback overwrote the
>> next insn? I'm not sure what behavior I would expect if that insn has already
>> been translated as part of the same tb. That said, the plugin is aware of which
>> insns have already been translated, so maybe it is not unreasonable to just
>> document this as a "don't do that". Let me know what you think.
>> 
>
> In the end, if we implement something to modify running code, we
> should make sure it works as expected (flushing the related tb). I am
> not sure about the current status, and all the changes that would be
> needed, but it's something we should discuss before implementing.

If the right access helpers are used we eventually end up in cputlb and
the code modification detection code will kick in. But that detection
mechanism relies on the guest controlled page tables marking executable
code and honouring rw permissions. If plugins don't honour those
permissions you'll become unstuck quite quickly.

> More globally, let's wait to hear feedback from maintainers to see if
> they are open to the idea or not. A "hard" no would end it there.

It's not a hard no - but I think any such patching ability would need a
quite a bit of thought to make sure edge cases are covered. However I do
expect there will be downstream forks that go further than the upstream
is currently comfortable with and if the code is structured in the right
way we can minimise the pain of re-basing.

>>> The hypercall functionality would be useful for plugins as a whole. And I think it definitely deserves to be worked on, if maintainers are open to that as well.
>> Sure, I'd be happy to work on this some more. At least on the
>> fuzzing side of
>> things, the way hypercalls are done across hypervisors (QEMU, Bochs, etc) is
>> pretty consistent so I think we could provide a useful common set of
>> functionality. The reason I did the bare minimum here is I'm not familiar with
>> every architecture, and a good NOP needs to be chosen for each one along with a
>> reasonable way to pass some arguments -- I don't know if I'm the right person
>> to make that call.
>> 
>
> We have been discussing something like that for system mode recently,
> so it would definitely be useful.
>
> IMHO, it's open for anyone to contribute this, the plugins area is not
> a private garden where only chosen ones can change things. Just be
> prepared for change requests, and multiple versions before the final
> one.
>
> Same on this one, we'll see if maintainers are ok with the idea.
>
>> Glad to hear you're for this idea!
>> -Rowan
>
> Thanks,
> Pierrick

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 0/3] Add additional plugin API functions to read and write memory and registers

[Pierrick Bouvier]

On 12/10/24 03:38, Alex Bennée wrote:
> Pierrick Bouvier <pierrick.bouvier@linaro.org> writes:
> 
>> On 12/6/24 16:57, Rowan Hart wrote:
>>>> I am personally in favor to adding such features in upstream QEMU,
>>>> but we should discuss it with the maintainers, because it would
>>>> allow to change the state of execution, which is something qemu
>>>> plugins actively didn't try to do. It's a real paradigm shift for
>>>> plugins.
>>>>
>>>> By writing to memory/registers, we can start replacing instructions and control flow, and there is a whole set of consequences to that.
>>>>
>>> Totally agree! As much as I really want this functionality for
>>> plugins, I think
>>> alignment on it is quite important. I can see very cool use cases for being
>>> able to replace instructions and control flow to allow hooking functions,
>>> hotpatching, and so forth.
> 
> I think currently that makes quite a lot of demands on the translator to
> make sure things are kept consistent.
> 
> We have been talking about maybe enabling hypercalls of some sort to
> allow for hooking explicit function boundaries in linux-user. A natural
> extension of that would be for host library bypass functions. I'm unsure
> of how that would apply in system emulation mode though where things are
> handled on a lot more granular level.
>

If we are talking about replacing library function calls with host 
variants, I'm not sure it's connected to the hypercalls we are talking 
about here.

>>> I don't really know the edge cases here so your expertise will be
>>> helpful. In
>>> the worst case I can imagine: what would happen if a callback overwrote the
>>> next insn? I'm not sure what behavior I would expect if that insn has already
>>> been translated as part of the same tb. That said, the plugin is aware of which
>>> insns have already been translated, so maybe it is not unreasonable to just
>>> document this as a "don't do that". Let me know what you think.
>>>
>>
>> In the end, if we implement something to modify running code, we
>> should make sure it works as expected (flushing the related tb). I am
>> not sure about the current status, and all the changes that would be
>> needed, but it's something we should discuss before implementing.
> 
> If the right access helpers are used we eventually end up in cputlb and
> the code modification detection code will kick in. But that detection
> mechanism relies on the guest controlled page tables marking executable
> code and honouring rw permissions. If plugins don't honour those
> permissions you'll become unstuck quite quickly.
> 
>> More globally, let's wait to hear feedback from maintainers to see if
>> they are open to the idea or not. A "hard" no would end it there.
> 
> It's not a hard no - but I think any such patching ability would need a
> quite a bit of thought to make sure edge cases are covered. However I do
> expect there will be downstream forks that go further than the upstream
> is currently comfortable with and if the code is structured in the right
> way we can minimise the pain of re-basing.
> 

In a more straightforward way, does it mean you are open to change state 
of execution through plugins?

Out of the technical aspect and getting all the corner cases right, we 
should first discuss if we want to go there, or if we decide it does not 
have its place upstream, and should belong to downstream forks instead.

>>>> The hypercall functionality would be useful for plugins as a whole. And I think it definitely deserves to be worked on, if maintainers are open to that as well.
>>> Sure, I'd be happy to work on this some more. At least on the
>>> fuzzing side of
>>> things, the way hypercalls are done across hypervisors (QEMU, Bochs, etc) is
>>> pretty consistent so I think we could provide a useful common set of
>>> functionality. The reason I did the bare minimum here is I'm not familiar with
>>> every architecture, and a good NOP needs to be chosen for each one along with a
>>> reasonable way to pass some arguments -- I don't know if I'm the right person
>>> to make that call.
>>>
>>
>> We have been discussing something like that for system mode recently,
>> so it would definitely be useful.
>>
>> IMHO, it's open for anyone to contribute this, the plugins area is not
>> a private garden where only chosen ones can change things. Just be
>> prepared for change requests, and multiple versions before the final
>> one.
>>
>> Same on this one, we'll see if maintainers are ok with the idea.
>>
>>> Glad to hear you're for this idea!
>>> -Rowan
>>
>> Thanks,
>> Pierrick
> 

Re: [PATCH v2 1/3] Expose gdb_write_register function to consumers of gdbstub

[Alex Bennée]

Rowan Hart <rowanbhart@gmail.com> writes:

> From: novafacing <rowanbhart@gmail.com>

You need to include Signed-of-by tags for anything to get accepted.

>
> ---
>  gdbstub/gdbstub.c      |  2 +-
>  include/exec/gdbstub.h | 14 ++++++++++++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index b1def7e71d..7d87a3324c 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -536,7 +536,7 @@ int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
>      return 0;
>  }
>  
> -static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
> +int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>      GDBRegisterState *r;
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index d73f424f56..584ed73fc9 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -118,6 +118,20 @@ const GDBFeature *gdb_find_static_feature(const char *xmlname);
>   */
>  int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
>  
> +/**
> + * gdb_write_register() - Write a register associated with a CPU.
> + * @cpu: The CPU associated with the register.
> + * @buf: The buffer that the register contents will be set to.
> + * @reg: The register's number returned by gdb_find_feature_register().
> + *
> + * The size of @buf must be at least the size of the register being
> + * written.
> + *
> + * Return: The number of written bytes, or 0 if an error occurred (for
> + * example, an unknown register was provided).
> + */
> +int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg);
> +
>  /**
>   * typedef GDBRegDesc - a register description from gdbstub
>   */

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH v2 2/3] Add plugin API functions for register R/W, hwaddr R/W, vaddr W

[Alex Bennée]

Rowan Hart <rowanbhart@gmail.com> writes:

> From: novafacing <rowanbhart@gmail.com>
>
> ---
>  include/qemu/qemu-plugin.h | 116 +++++++++++++++++++++++++++++++++----
>  plugins/api.c              |  66 ++++++++++++++++++++-
>  2 files changed, 168 insertions(+), 14 deletions(-)
>
> diff --git a/include/qemu/qemu-plugin.h b/include/qemu/qemu-plugin.h
> index 0fba36ae02..b812593e7f 100644
> --- a/include/qemu/qemu-plugin.h
> +++ b/include/qemu/qemu-plugin.h
> @@ -65,11 +65,18 @@ typedef uint64_t qemu_plugin_id_t;
>   *
>   * version 4:
>   * - added qemu_plugin_read_memory_vaddr
> + *
> + * version 5:
> + * - added qemu_plugin_write_memory_vaddr
> + * - added qemu_plugin_read_memory_hwaddr
> + * - added qemu_plugin_write_memory_hwaddr
> + * - added qemu_plugin_write_register
> + *
>   */

In the next version please split up the API additions to make it easier
to review. For now w.r.t. hwaddr I refer to:

  Subject: Re: [PATCH 1/1] plugins: add API to read guest CPU memory from hwaddr
  In-Reply-To: <20240828063224.291503-2-rowanbhart@gmail.com> (Rowan Hart's message of "Tue, 27 Aug 2024 23:32:24 -0700")
  References: <20240828063224.291503-1-rowanbhart@gmail.com> <20240828063224.291503-2-rowanbhart@gmail.com>
  Date: Thu, 09 Jan 2025 11:38:48 +0000
  Message-ID: <877c749pyv.fsf@draig.linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro