1. Patchew
  2. Xen
  3. View series

[PATCH] x86/hvm: Fix fast singlestep state persistence

Petr Beneš posted 1 patch 2 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/fa519b9a544d3d19a31313292a909d12775e6e1f.1707427103.git.w1benny@gmail.com
xen/arch/x86/hvm/hvm.c | 32 +++++++++++++++++++++++---------
1 file changed, 23 insertions(+), 9 deletions(-)
[PATCH] x86/hvm: Fix fast singlestep state persistence
Posted by Petr Beneš 2 months, 3 weeks ago 
From: Petr Beneš <w1benny@gmail.com>

This patch addresses an issue where the fast singlestep setting would persist
despite xc_domain_debug_control being called with XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF.
Specifically, if fast singlestep was enabled in a VMI session and that session
stopped before the MTF trap occurred, the fast singlestep setting remained
active even though MTF itself was disabled.  This led to a situation where, upon
starting a new VMI session, the first event to trigger an EPT violation would
cause the corresponding EPT event callback to be skipped due to the lingering
fast singlestep setting.

The fix ensures that the fast singlestep setting is properly reset when
disabling single step debugging operations.

Signed-off-by: Petr Beneš <w1benny@gmail.com>
---
 xen/arch/x86/hvm/hvm.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e8deeb0222..4f988de4c1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5160,26 +5160,40 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
 int hvm_debug_op(struct vcpu *v, int32_t op)
 {
-    int rc;
+    int rc = 0;
 
     switch ( op )
     {
         case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON:
         case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF:
-            rc = -EOPNOTSUPP;
             if ( !cpu_has_monitor_trap_flag )
-                break;
-            rc = 0;
-            vcpu_pause(v);
-            v->arch.hvm.single_step =
-                (op == XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON);
-            vcpu_unpause(v); /* guest will latch new state */
+                return -EOPNOTSUPP;
             break;
         default:
-            rc = -ENOSYS;
+            return -ENOSYS;
+    }
+
+    vcpu_pause(v);
+
+    switch ( op )
+    {
+        case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON:
+            v->arch.hvm.single_step = true;
+            break;
+
+        case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF:
+            v->arch.hvm.single_step = false;
+            v->arch.hvm.fast_single_step.enabled = false;
+            v->arch.hvm.fast_single_step.p2midx = 0;
             break;
+
+        default:
+            ASSERT_UNREACHABLE();
+            return -ENOSYS;
     }
 
+    vcpu_unpause(v); /* guest will latch new state */
+
     return rc;
 }
 
-- 
2.34.1
Re: [PATCH] x86/hvm: Fix fast singlestep state persistence
Posted by Andrew Cooper 2 months, 3 weeks ago 
On 08/02/2024 9:20 pm, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
>
> This patch addresses an issue where the fast singlestep setting would persist
> despite xc_domain_debug_control being called with XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF.
> Specifically, if fast singlestep was enabled in a VMI session and that session
> stopped before the MTF trap occurred, the fast singlestep setting remained
> active even though MTF itself was disabled.  This led to a situation where, upon
> starting a new VMI session, the first event to trigger an EPT violation would
> cause the corresponding EPT event callback to be skipped due to the lingering
> fast singlestep setting.
>
> The fix ensures that the fast singlestep setting is properly reset when
> disabling single step debugging operations.
>
> Signed-off-by: Petr Beneš <w1benny@gmail.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index e8deeb0222..4f988de4c1 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5160,26 +5160,40 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>  
>  int hvm_debug_op(struct vcpu *v, int32_t op)
>  {
> -    int rc;
> +    int rc = 0;
>  
>      switch ( op )
>      {
>          case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON:
>          case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF:
> -            rc = -EOPNOTSUPP;
>              if ( !cpu_has_monitor_trap_flag )
> -                break;
> -            rc = 0;
> -            vcpu_pause(v);
> -            v->arch.hvm.single_step =
> -                (op == XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON);
> -            vcpu_unpause(v); /* guest will latch new state */
> +                return -EOPNOTSUPP;
>              break;
>          default:
> -            rc = -ENOSYS;
> +            return -ENOSYS;
> +    }
> +
> +    vcpu_pause(v);
> +
> +    switch ( op )
> +    {
> +        case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON:
> +            v->arch.hvm.single_step = true;
> +            break;
> +
> +        case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF:
> +            v->arch.hvm.single_step = false;
> +            v->arch.hvm.fast_single_step.enabled = false;
> +            v->arch.hvm.fast_single_step.p2midx = 0;
>              break;
> +
> +        default:
> +            ASSERT_UNREACHABLE();

Two things.

First, this reads as if it's reachable, and therefore wrong.  You
probably want an /* Excluded above */ comment to point out why it's safe
in this case.

Second, I know you're copying the existing switch(), but it wasn't
compliant with Xen's coding style.  The cases and their clauses should
have one fewer indentation level.

I'm happy to fix up both on commit.

~Andrew
Re: [PATCH] x86/hvm: Fix fast singlestep state persistence
Posted by Petr Beneš 2 months, 3 weeks ago 
> On Fri, Feb 9, 2024 at 3:58 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 08/02/2024 9:20 pm, Petr Beneš wrote:
> > From: Petr Beneš <w1benny@gmail.com>
> >
> > This patch addresses an issue where the fast singlestep setting would persist
> > despite xc_domain_debug_control being called with XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF.
> > Specifically, if fast singlestep was enabled in a VMI session and that session
> > stopped before the MTF trap occurred, the fast singlestep setting remained
> > active even though MTF itself was disabled.  This led to a situation where, upon
> > starting a new VMI session, the first event to trigger an EPT violation would
> > cause the corresponding EPT event callback to be skipped due to the lingering
> > fast singlestep setting.
> >
> > The fix ensures that the fast singlestep setting is properly reset when
> > disabling single step debugging operations.
> >
> > Signed-off-by: Petr Beneš <w1benny@gmail.com>
> > ---
> >  xen/arch/x86/hvm/hvm.c | 32 +++++++++++++++++++++++---------
> >  1 file changed, 23 insertions(+), 9 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index e8deeb0222..4f988de4c1 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -5160,26 +5160,40 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> >
> >  int hvm_debug_op(struct vcpu *v, int32_t op)
> >  {
> > -    int rc;
> > +    int rc = 0;
> >
> >      switch ( op )
> >      {
> >          case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON:
> >          case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF:
> > -            rc = -EOPNOTSUPP;
> >              if ( !cpu_has_monitor_trap_flag )
> > -                break;
> > -            rc = 0;
> > -            vcpu_pause(v);
> > -            v->arch.hvm.single_step =
> > -                (op == XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON);
> > -            vcpu_unpause(v); /* guest will latch new state */
> > +                return -EOPNOTSUPP;
> >              break;
> >          default:
> > -            rc = -ENOSYS;
> > +            return -ENOSYS;
> > +    }
> > +
> > +    vcpu_pause(v);
> > +
> > +    switch ( op )
> > +    {
> > +        case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_ON:
> > +            v->arch.hvm.single_step = true;
> > +            break;
> > +
> > +        case XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF:
> > +            v->arch.hvm.single_step = false;
> > +            v->arch.hvm.fast_single_step.enabled = false;
> > +            v->arch.hvm.fast_single_step.p2midx = 0;
> >              break;
> > +
> > +        default:
> > +            ASSERT_UNREACHABLE();
>
> Two things.
>
> First, this reads as if it's reachable, and therefore wrong.  You
> probably want an /* Excluded above */ comment to point out why it's safe
> in this case.
>
> Second, I know you're copying the existing switch(), but it wasn't
> compliant with Xen's coding style.  The cases and their clauses should
> have one fewer indentation level.
>
> I'm happy to fix up both on commit.
>
> ~Andrew

Thanks for the feedback. If it's not too much of a hassle, I'll be
happy if you fix it.

P.
Re: [PATCH] x86/hvm: Fix fast singlestep state persistence
Posted by Tamas K Lengyel 2 months, 3 weeks ago 
On Thu, Feb 8, 2024 at 4:20 PM Petr Beneš <w1benny@gmail.com> wrote:
>
> From: Petr Beneš <w1benny@gmail.com>
>
> This patch addresses an issue where the fast singlestep setting would persist
> despite xc_domain_debug_control being called with XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF.
> Specifically, if fast singlestep was enabled in a VMI session and that session
> stopped before the MTF trap occurred, the fast singlestep setting remained
> active even though MTF itself was disabled.  This led to a situation where, upon
> starting a new VMI session, the first event to trigger an EPT violation would
> cause the corresponding EPT event callback to be skipped due to the lingering
> fast singlestep setting.
>
> The fix ensures that the fast singlestep setting is properly reset when
> disabling single step debugging operations.
>
> Signed-off-by: Petr Beneš <w1benny@gmail.com>

Thanks, this has been a known bug that awaited a fix for a long time.

Reviewed-by: Tamas K Lengyel <tamas@tklengyel.com>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.