[v1] target/arm: Check if debug is already initialized

[PATCH] target/arm: Check if debug is already initialized

Posted by Akihiko Odaki 1 year, 1 month ago

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);
+    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)) {
-- 
2.40.0

Re: [PATCH] target/arm: Check if debug is already initialized

Posted by Peter Maydell 1 year, 1 month ago

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

Re: [PATCH] target/arm: Check if debug is already initialized

Posted by Philippe Mathieu-Daudé 1 year, 1 month ago

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.