From: y zhou <[email protected]>
The opaque register handles returned by qemu_plugin_get_registers()
are currently encoded as:
handle = (gdb_reg << 1) | read_only_bit
For the first core register of every target -- gdb_reg 0, not
read-only -- this encodes to GINT_TO_POINTER(0), i.e. NULL. That is
x86_64 "rax", aarch64 "x0" and ppc "r0", among others.
A NULL handle is indistinguishable from an invalid one, so any plugin
that defensively checks its handles silently loses access to the
first core register (where the Linux syscall number lives on x86_64,
for example). Passing NULL to qemu_plugin_write_register() instead of
being rejected silently writes register 0.
Restore the invariant that NULL is never a valid handle by keeping an
offset on the gdb register number, as was already done before the
read-only property was packed into the handle (see v10.0.0, where
handles were encoded as GINT_TO_POINTER(gdb_reg + 1)):
handle = ((gdb_reg + 1) << 1) | read_only_bit
Also assert in tests/tcg/plugins/registers.c that every descriptor
carries a non-NULL handle: the current test passes the handles
straight to read/write without checking them, and a handle encoding
that maps a valid register to NULL goes unnoticed for that reason.
Found while writing a system-mode syscall tracing plugin, which could
read neither the syscall number on x86_64 nor the first argument on
aarch64 (both handles came back NULL). Auditing every
GINT_TO_POINTER/GPOINTER_TO_INT site in the tree shows this is the
only place where a valid entity encodes to NULL.
Fixes: 55327b85ce32c ("plugins: prohibit writing to read-only registers")
Signed-off-by: y zhou <[email protected]>
Cc: Alex Bennée <[email protected]>
Cc: Pierrick Bouvier <[email protected]>
Cc: Alexandre Iooss <[email protected]>
Cc: [email protected]
Reviewed-by: Pierrick Bouvier <[email protected]>
Link:
https://lore.kernel.org/qemu-devel/[email protected]
Signed-off-by: Pierrick Bouvier <[email protected]>
---
plugins/api.c | 7 ++++---
tests/tcg/plugins/registers.c | 8 ++++++++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/plugins/api.c b/plugins/api.c
index 3520c96f411..46cba6796cc 100644
--- a/plugins/api.c
+++ b/plugins/api.c
@@ -455,7 +455,7 @@ static GArray *create_register_handles(GArray *gdbstub_regs)
desc.is_readonly = true;
plugin_ro_bit = 1;
}
- desc.handle = GINT_TO_POINTER((grd->gdb_reg << 1) | plugin_ro_bit);
+ desc.handle = GINT_TO_POINTER(((grd->gdb_reg + 1) << 1) |
plugin_ro_bit);
desc.feature = g_intern_string(grd->feature_name);
g_array_append_val(find_data, desc);
}
@@ -480,7 +480,7 @@ bool qemu_plugin_read_register(struct qemu_plugin_register
*reg,
return false;
}
- return (gdb_read_register(current_cpu, buf, GPOINTER_TO_INT(reg) >> 1) >
0);
+ return (gdb_read_register(current_cpu, buf, (GPOINTER_TO_INT(reg) >> 1) -
1) > 0);
}
bool qemu_plugin_write_register(struct qemu_plugin_register *reg,
@@ -497,7 +497,8 @@ bool qemu_plugin_write_register(struct qemu_plugin_register
*reg,
return false;
}
- return (gdb_write_register(current_cpu, buf->data, GPOINTER_TO_INT(reg) >>
1) > 0);
+ return (gdb_write_register(current_cpu, buf->data,
+ (GPOINTER_TO_INT(reg) >> 1) - 1) > 0);
}
void qemu_plugin_set_pc(uint64_t vaddr)
diff --git a/tests/tcg/plugins/registers.c b/tests/tcg/plugins/registers.c
index 0e41734435f..02058fe8a82 100644
--- a/tests/tcg/plugins/registers.c
+++ b/tests/tcg/plugins/registers.c
@@ -29,6 +29,14 @@ static void vcpu_init_cb(unsigned int vcpu_index, void
*userdata)
qemu_plugin_reg_descriptor *reg_desc = NULL;
bool success = false;
+ /* NULL must never be a valid register handle */
+ for (size_t i = 0; i < regs->len; i++) {
+ qemu_plugin_reg_descriptor *desc =
+ &g_array_index(regs, qemu_plugin_reg_descriptor, i);
+
+ g_assert(desc->handle != NULL);
+ }
+
/* Make sure we can read and write a register not marked as readonly */
for (size_t i = 0; i < regs->len; i++) {
reg_desc = &g_array_index(regs, qemu_plugin_reg_descriptor, i);
--
2.43.0
