From de9cb2fb2773c6b2765380e8cb863a4e975167ce Mon Sep 17 00:00:00 2001 From: Daniel Roethlisberger Date: Sat, 20 Dec 2025 22:53:06 +0100 Subject: [PATCH 1/2] Avoid memory corruption in arm context save/restore code Addresses multiple issues in the arm context save/restore code: - When restoring from context, do not clobber pointers in CPUARMState with data from the context buffer. This causes use-after-free bugs usually manifesting as double-free crashes. - When saving to context, do not copy pointers from CPUARMState to the context buffer, to avoid leaking pointers to the context. Instead, zero the respective area of the context buffer, to ensure that reg_read and reg_write can access fields outside of the copied range. Specifically, env->uc needs to be NULL when accessed for a context. - When restoring to a CPU with a different nr than the source CPU, copy min(nr, ctx_nr) instead of nothing at all. - Add regression tests checking all CPU models for arm and arm64 for trivial double-free crashes. Fixes unicorn-engine/unicorn#2195 --- qemu/target/arm/unicorn_arm.c | 29 +++++++++++++++++++---------- tests/unit/test_arm.c | 26 ++++++++++++++++++++++++++ tests/unit/test_arm64.c | 26 ++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/qemu/target/arm/unicorn_arm.c b/qemu/target/arm/unicorn_arm.c index 7ecd0416d6..f0c502dade 100644 --- a/qemu/target/arm/unicorn_arm.c +++ b/qemu/target/arm/unicorn_arm.c @@ -630,9 +630,18 @@ static int arm_cpus_init(struct uc_struct *uc, const char *cpu_model) return 0; } +// The strategy is to copy the top part of CPUARMState that does not contain +// any pointers into the context buffer, and zero-fill the remainder. For all +// fields in the pointer range, data that needs to be saved to the context must +// be manually appended to the end of the buffer. We need to zero the pointer +// portion in the context buffer, because reg_read/reg_write are accessing some +// of these fields when accessing registers in a context. +#define CONTEXT_COPYSIZE offsetof(CPUARMState, pmsav7) +#define CONTEXT_ZEROSIZE (sizeof(CPUARMState) - CONTEXT_COPYSIZE) + static size_t uc_arm_context_size(struct uc_struct *uc) { - size_t ret = offsetof(CPUARMState, cpu_watchpoint); + size_t ret = CONTEXT_COPYSIZE + CONTEXT_ZEROSIZE; ARMCPU *cpu = (ARMCPU *)uc->cpu; CPUARMState *env = (CPUARMState *)&cpu->env; uint32_t nr; @@ -708,8 +717,10 @@ static uc_err uc_arm_context_save(struct uc_struct *uc, uc_context *context) p += sizeof(uint32_t) * nr; \ } p = context->data; - memcpy(p, uc->cpu->env_ptr, uc->cpu_context_size); - p += uc->cpu_context_size; + memcpy(p, uc->cpu->env_ptr, CONTEXT_COPYSIZE); + p += CONTEXT_COPYSIZE; + memset(p, 0, CONTEXT_ZEROSIZE); + p += CONTEXT_ZEROSIZE; nr = cpu->pmsav7_dregion; ARM_ENV_SAVE(env->pmsav7.drbar) @@ -739,8 +750,8 @@ static uc_err uc_arm_context_restore(struct uc_struct *uc, uc_context *context) ctx_nr = *(uint32_t *)p; \ if (ctx_nr != 0) { \ p += sizeof(uint32_t); \ - if (field && ctx_nr == nr) { \ - memcpy(field, p, sizeof(uint32_t) * ctx_nr); \ + if (field && nr != 0) { \ + memcpy(field, p, sizeof(uint32_t) * (ctx_nr > nr ? nr : ctx_nr)); \ } \ p += sizeof(uint32_t) * ctx_nr; \ } else { \ @@ -748,8 +759,8 @@ static uc_err uc_arm_context_restore(struct uc_struct *uc, uc_context *context) } p = context->data; - memcpy(uc->cpu->env_ptr, p, uc->cpu_context_size); - p += uc->cpu_context_size; + memcpy(uc->cpu->env_ptr, p, CONTEXT_COPYSIZE); + p += CONTEXT_COPYSIZE + CONTEXT_ZEROSIZE; nr = cpu->pmsav7_dregion; ARM_ENV_RESTORE(env->pmsav7.drbar) @@ -765,8 +776,6 @@ static uc_err uc_arm_context_restore(struct uc_struct *uc, uc_context *context) ARM_ENV_RESTORE(env->sau.rlar) #undef ARM_ENV_RESTORE - // Overwrite uc to our uc - env->uc = uc; return UC_ERR_OK; } @@ -793,7 +802,7 @@ void uc_init(struct uc_struct *uc) uc->cpus_init = arm_cpus_init; uc->insn_hook_validate = arm_insn_hook_validate; uc->opcode_hook_invalidate = arm_opcode_hook_invalidate; - uc->cpu_context_size = offsetof(CPUARMState, cpu_watchpoint); + uc->cpu_context_size = CONTEXT_COPYSIZE + CONTEXT_ZEROSIZE; uc->context_size = uc_arm_context_size; uc->context_save = uc_arm_context_save; uc->context_restore = uc_arm_context_restore; diff --git a/tests/unit/test_arm.c b/tests/unit/test_arm.c index 3b0c7915cb..438aaa4e65 100644 --- a/tests/unit/test_arm.c +++ b/tests/unit/test_arm.c @@ -1062,6 +1062,31 @@ static void test_arm_hook_insn_wfi(void) OK(uc_close(uc)); } +static void test_arm_context(void) +{ + for (int model = 0; model < UC_CPU_ARM_ENDING; model++) { + uc_engine *uc; + uc_engine *uc2; + uc_context *ctx; + + OK(uc_open(UC_ARCH_ARM, UC_MODE_ARM, &uc)); + OK(uc_ctl_set_cpu_model(uc, model)); + + OK(uc_context_alloc(uc, &ctx)); + OK(uc_context_save(uc, ctx)); + + OK(uc_open(UC_ARCH_ARM, UC_MODE_ARM, &uc2)); + OK(uc_ctl_set_cpu_model(uc2, model)); + + OK(uc_context_restore(uc2, ctx)); + + // One of these will abort on a double-free for bug-afflicted CPU models. + OK(uc_context_free(ctx)); + OK(uc_close(uc)); + OK(uc_close(uc2)); + } +} + TEST_LIST = {{"test_arm_nop", test_arm_nop}, {"test_arm_thumb_sub", test_arm_thumb_sub}, {"test_armeb_sub", test_armeb_sub}, @@ -1094,4 +1119,5 @@ TEST_LIST = {{"test_arm_nop", test_arm_nop}, {"test_arm_v7_lpae", test_arm_v7_lpae}, {"test_arm_svc_hvc_syndrome", test_arm_svc_hvc_syndrome}, {"test_arm_hook_insn_wfi", test_arm_hook_insn_wfi}, + {"test_arm_context", test_arm_context}, {NULL, NULL}}; diff --git a/tests/unit/test_arm64.c b/tests/unit/test_arm64.c index 68db50ac21..98f92d5407 100644 --- a/tests/unit/test_arm64.c +++ b/tests/unit/test_arm64.c @@ -944,6 +944,31 @@ static void test_arm64_pauth_ctl(void) OK(uc_close(uc)); } +static void test_arm64_context(void) +{ + for (int model = 0; model < UC_CPU_ARM64_ENDING; model++) { + uc_engine *uc; + uc_engine *uc2; + uc_context *ctx; + + OK(uc_open(UC_ARCH_ARM64, UC_MODE_ARM, &uc)); + OK(uc_ctl_set_cpu_model(uc, model)); + + OK(uc_context_alloc(uc, &ctx)); + OK(uc_context_save(uc, ctx)); + + OK(uc_open(UC_ARCH_ARM64, UC_MODE_ARM, &uc2)); + OK(uc_ctl_set_cpu_model(uc2, model)); + + OK(uc_context_restore(uc2, ctx)); + + // One of these will abort on a double-free for bug-afflicted CPU models. + OK(uc_context_free(ctx)); + OK(uc_close(uc)); + OK(uc_close(uc2)); + } +} + TEST_LIST = {{"test_arm64_until", test_arm64_until}, {"test_arm64_code_patching", test_arm64_code_patching}, {"test_arm64_code_patching_count", test_arm64_code_patching_count}, @@ -965,4 +990,5 @@ TEST_LIST = {{"test_arm64_until", test_arm64_until}, {"test_arm64_pc_guarantee", test_arm64_pc_guarantee}, {"test_arm64_pauth_vanilla", test_arm64_pauth_vanilla}, {"test_arm64_pauth_ctl", test_arm64_pauth_ctl}, + {"test_arm64_context", test_arm64_context}, {NULL, NULL}}; From cd473c7160bd3aa9d51f38bb49bfe82570f92a0b Mon Sep 17 00:00:00 2001 From: Daniel Roethlisberger Date: Sun, 21 Dec 2025 14:14:00 +0100 Subject: [PATCH 2/2] Add cautionary comment for AArch64 uc->cpu_context_size --- qemu/target/arm/unicorn_aarch64.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/qemu/target/arm/unicorn_aarch64.c b/qemu/target/arm/unicorn_aarch64.c index 82ea40c99b..cc3c79fc24 100644 --- a/qemu/target/arm/unicorn_aarch64.c +++ b/qemu/target/arm/unicorn_aarch64.c @@ -533,6 +533,11 @@ void uc_init(struct uc_struct *uc) uc->release = arm64_release; uc->cpus_init = arm64_cpus_init; uc->insn_hook_validate = arm64_insn_hook_validate; + // n.b. The following results in memcpy of pointers from CPUARMState to + // context buffers and vice versa. That is currently safe for AArch64 + // because none of the pointer fields of CPUARMState are allocated for any + // of the AArch64 CPUs, and none of reg_read/reg_write, when called on a + // context, accesses fields including and beyond cpu_watchpoint. uc->cpu_context_size = offsetof(CPUARMState, cpu_watchpoint); uc->pauth_sign = arm64_pauth_sign; uc->pauth_strip = arm64_pauth_strip;