On 3/4/26 9:56 AM, Pierrick Bouvier wrote:
On 3/4/26 8:18 AM, Florian Hofhammer wrote:
+static bool vcpu_syscall_filter(qemu_plugin_id_t id, unsigned int vcpu_index,
+ int64_t num, uint64_t a1, uint64_t a2,
+ uint64_t a3, uint64_t a4, uint64_t a5,
+ uint64_t a6, uint64_t a7, uint64_t a8,
+ uint64_t *sysret)
+{
+ if (num == 4095) {
+ qemu_plugin_outs("Communication syscall detected, set target_pc / "
+ "target_vaddr\n");
+ source_pc = a1;
+ target_pc = a2;
+ target_vaddr = a3;
+ if (source_pc >> 63 || target_pc >> 63 || target_vaddr >> 63) {
+ /*
+ * Some architectures (e.g., m68k) use 32-bit addresses with the
+ * top bit set, which causes them to get sign-extended somewhere in
+ * the chain to this callback. We mask the top bits off here to get
+ * the actual addresses.
+ */
This looks like an actual bug. Using a backtrace here, can you find which
function extended it?
Parameter should be treated as unsigned everywhere, else something is really
going wrong.
tl;dr: register values are considered signed in the syscall handling
code, which seems incorrect to me. Details below.
This seems to be a bigger issue in the syscall handling code and I'm
surprised this never surfaced before. Generally, the CPUArchState struct
in target/*/cpu.h defines the registers as unsigned types. When a
syscall is encountered, the main cpu loop calls do_syscall() from
linux-user/syscall.c, which takes and returns abi_long values. abi_long
is defined as either int32_t or target_long in include/user/abitypes.h
and therefore a signed type, so the register values get converted from
unsigned to signed types here. do_syscall() passes those abi_long values
on, e.g., to record_syscall_start() and send_through_syscall_filters(),
which still take the register values in as abi_long. Those in turn
however pass the values on to qemu_plugin_vcpu_syscall() and
qemu_plugin_vcpu_syscall_filter(), respectively. Those two functions now
take uint64_t values for the arguments, so for 32-bit architectures the
conversion chain is uint32_t (CPUArchState) -> int32_t (do_syscall) ->
uint64_t (callbacks).
This seems to be some bigger refactoring changing all those abi_longs to
abi_ulongs. Would you like me to prepare a separate patch series for
this?
Indeed, they are signed in linux-user, so maybe my original assumption
that it *should* be unsigned is wrong. Let me take a look to see what
should be the correct semantic here.
As a follow-up on this, changing this seems to be a bit more complicated
than I initially thought. The arguments could be simply made unsigned,
but the return value (which is also just a register content and should
therefore be unsigned if I understand correctly) can't easily be made
unsigned. The per-target main loop relies on the return value of
do_syscall() to be signed to determine whether one of the QEMU-specific
errors (e.g., QEMU_ERESTARTSYS) was returned.
Note: some architectures already define the return value as unsigned in
the main cpu loop and implicitly cast the return value of do_syscall().
Given that in theory, a syscall's return value could always coincide
with the values of QEMU's special errors, I'm wondering whether it would
make sense to make do_syscall()'s return value unsigned and signal those
special error codes out-of-band instead of in the syscall return value,
e.g., either via a separate pointer argument to do_syscall() or by
making do_syscall() return a struct containing both the return value and
a potential error code.
It could be a solution. I'm just a little bit relunctant to make such a
change now because we have limited time (soft freeze is coming next
week), so not the right time to make such deep changes and... it has
been working like that for long time.
But maybe I'm getting ahead of myself here, please let me know what you
think!
Thanks for your very good analysis. I'll come back once I have more
information about what should be the correct fix.
Meanwhile, and because we are close from softfreeze, just keep the
current hack in the test. This way, we can focus on sending your series
and the additional comment patch you sent.
Thanks,
Pierrick
The attached patch fixes the problem by clamping syscall arguments for
32-bit targets. Return type has to be signed anyway, so no change is
needed on this side.
You can add it as the first patch on your next version, and remove the
hack checking the highest bit in this test, now that the problem is solved.
Regards,
Pierrick
From 9814b90693bb5a6949f1bcdc9a31bef558fc5539 Mon Sep 17 00:00:00 2001
From: Pierrick Bouvier <[email protected]>
Date: Wed, 4 Mar 2026 11:18:35 -0800
Subject: [PATCH] plugins/core: clamp syscall arguments if target is 32-bit
Syscall arguments are abi_long in user code, and plugin syscall
interface works with uint64_t only.
According to C integer promotion rules, the value is sign extended
before becoming unsigned, thus setting high bits when only 32-bit lower
ones should have a significant value.
As a result, we need to clamp values we receive from user-code
accordingly.
Signed-off-by: Pierrick Bouvier <[email protected]>
---
plugins/core.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/plugins/core.c b/plugins/core.c
index 7220b9dbb4a..2324bbffa3d 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -516,6 +516,23 @@ void qemu_plugin_tb_trans_cb(CPUState *cpu, struct qemu_plugin_tb *tb)
}
}
+static void clamp_syscall_arguments(uint64_t *a1, uint64_t *a2, uint64_t *a3,
+ uint64_t *a4, uint64_t *a5, uint64_t *a6,
+ uint64_t *a7, uint64_t *a8)
+{
+ if (target_long_bits() == 32) {
+ const uint64_t mask = UINT32_MAX;
+ *a1 &= mask;
+ *a2 &= mask;
+ *a3 &= mask;
+ *a4 &= mask;
+ *a5 &= mask;
+ *a6 &= mask;
+ *a7 &= mask;
+ *a8 &= mask;
+ }
+}
+
/*
* Disable CFI checks.
* The callback function has been loaded from an external library so we do not
@@ -534,6 +551,8 @@ qemu_plugin_vcpu_syscall(CPUState *cpu, int64_t num, uint64_t a1, uint64_t a2,
return;
}
+ clamp_syscall_arguments(&a1, &a2, &a3, &a4, &a5, &a6, &a7, &a8);
+
QLIST_FOREACH_SAFE_RCU(cb, &plugin.cb_lists[ev], entry, next) {
qemu_plugin_vcpu_syscall_cb_t func = cb->f.vcpu_syscall;
@@ -587,6 +606,8 @@ qemu_plugin_vcpu_syscall_filter(CPUState *cpu, int64_t num, uint64_t a1,
return false;
}
+ clamp_syscall_arguments(&a1, &a2, &a3, &a4, &a5, &a6, &a7, &a8);
+
qemu_plugin_set_cb_flags(cpu, QEMU_PLUGIN_CB_RW_REGS_PC);
QLIST_FOREACH_SAFE_RCU(cb, &plugin.cb_lists[ev], entry, next) {
--
2.47.3
