| Message ID | 20230405070244.23464-1-akihiko.odaki@daynix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/arm: Check if debug is already initialized | expand |
On 5/4/23 09:02, Akihiko Odaki wrote: > When virtualizing SMP system, kvm_arm_init_debug() will be called > multiple times. Check if the debug feature is already initialized when the > function is called; otherwise it will overwrite pointers to memory > allocated with the previous call and leak it. > > Fixes: e4482ab7e3 ("target-arm: kvm - add support for HW assisted debug") > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > target/arm/kvm64.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index 1197253d12..d2fce5e582 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -32,7 +32,11 @@ > #include "hw/acpi/ghes.h" > #include "hw/arm/virt.h" > > -static bool have_guest_debug; > +static enum { > + GUEST_DEBUG_UNINITED, > + GUEST_DEBUG_INITED, > + GUEST_DEBUG_UNAVAILABLE, > +} guest_debug; > > /* > * Although the ARM implementation of hardware assisted debugging > @@ -84,8 +88,14 @@ GArray *hw_breakpoints, *hw_watchpoints; > */ > static void kvm_arm_init_debug(CPUState *cs) > { > - have_guest_debug = kvm_check_extension(cs->kvm_state, > - KVM_CAP_SET_GUEST_DEBUG); - Maybe we can merge kvm{,64}.c (see commit 82bf7ae84c "target/arm: Remove KVM support for 32-bit Arm hosts") - Could kvm_arm_init_debug() belong to kvm_arch_init()? Then this patch / enum is not required. - Why we keep a reference to the global kvm_state in CPUState is not clear to me.
On Wed, 5 Apr 2023 at 08:02, Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > When virtualizing SMP system, kvm_arm_init_debug() will be called > multiple times. Check if the debug feature is already initialized when the > function is called; otherwise it will overwrite pointers to memory > allocated with the previous call and leak it. > > Fixes: e4482ab7e3 ("target-arm: kvm - add support for HW assisted debug") > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > target/arm/kvm64.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) I think I agree with Philippe that the better fix is to call kvm_arm_init_debug() from kvm_arch_init() -- if we avoid calling this for each vcpu then we don't have to carefully arrange to ignore all but the first call. We never actually care about the CPUState we're passed in, so we could instead pass in the KVMState directly, which kvm_arch_init() has. thanks -- PMM
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c index 1197253d12..d2fce5e582 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -32,7 +32,11 @@ #include "hw/acpi/ghes.h" #include "hw/arm/virt.h" -static bool have_guest_debug; +static enum { + GUEST_DEBUG_UNINITED, + GUEST_DEBUG_INITED, + GUEST_DEBUG_UNAVAILABLE, +} guest_debug; /* * Although the ARM implementation of hardware assisted debugging @@ -84,8 +88,14 @@ GArray *hw_breakpoints, *hw_watchpoints; */ static void kvm_arm_init_debug(CPUState *cs) { - have_guest_debug = kvm_check_extension(cs->kvm_state, - KVM_CAP_SET_GUEST_DEBUG); + if (guest_debug) { + return; + } + + if (!kvm_check_extension(cs->kvm_state, KVM_CAP_SET_GUEST_DEBUG)) { + guest_debug = GUEST_DEBUG_UNAVAILABLE; + return; + } max_hw_wps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_WPS); hw_watchpoints = g_array_sized_new(true, true, @@ -94,7 +104,8 @@ static void kvm_arm_init_debug(CPUState *cs) max_hw_bps = kvm_check_extension(cs->kvm_state, KVM_CAP_GUEST_DEBUG_HW_BPS); hw_breakpoints = g_array_sized_new(true, true, sizeof(HWBreakpoint), max_hw_bps); - return; + + guest_debug = GUEST_DEBUG_INITED; } /** @@ -1483,7 +1494,7 @@ static const uint32_t brk_insn = 0xd4200000; int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) { - if (have_guest_debug) { + if (guest_debug == GUEST_DEBUG_INITED) { if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) || cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk_insn, 4, 1)) { return -EINVAL; @@ -1499,7 +1510,7 @@ int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp) { static uint32_t brk; - if (have_guest_debug) { + if (guest_debug == GUEST_DEBUG_INITED) { if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&brk, 4, 0) || brk != brk_insn || cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 1)) {
When virtualizing SMP system, kvm_arm_init_debug() will be called multiple times. Check if the debug feature is already initialized when the function is called; otherwise it will overwrite pointers to memory allocated with the previous call and leak it. Fixes: e4482ab7e3 ("target-arm: kvm - add support for HW assisted debug") Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- target/arm/kvm64.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)