| Message ID | a677e1cb-3a81-b42c-25c1-ab2b07d8996a@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/debug: fix guest dr6 value for single stepping and HW breakpoints | expand |
On 8/19/23 00:47, Jinoh Kang wrote: > Today, when a HVM (or PVH) guest triggers a hardware breakpoint while > EFLAGS.TF is set, Xen incorrectly assumes that this is a single stepping > exception and sets DR_STEP in dr6 in addition to DR_TRAP<n>. > > This causes problems with Linux HW breakpoint handler, which ignores > DR_TRAP<n> bits when DR_STEP is set. This prevents user-mode debuggers > from recognizing hardware breakpoints if EFLAGS.TF is set. > > Fix this by not setting DR_STEP in {vmx,svm}_inject_event, unless the > emulator explicitly signals the single-stepping mode via the newly added > "singlestep" boolean field of struct x86_event. s/the newly added "singlestep" boolean field/the 'extra' field/. oops. > > Fixes: 8b831f4189 ("x86: single step after instruction emulation") > Signed-off-by: Jinoh Kang <jinoh.kang.kr@gmail.com> > --- > xen/arch/x86/hvm/emulate.c | 3 ++- > xen/arch/x86/hvm/svm/svm.c | 6 +++--- > xen/arch/x86/hvm/vmx/vmx.c | 6 +++--- > xen/arch/x86/include/asm/hvm/hvm.h | 12 ++++++++++++ > xen/arch/x86/mm/shadow/multi.c | 5 +++-- > xen/arch/x86/x86_emulate/x86_emulate.h | 4 +++- > 6 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 9b6e4c8bc61b..5ad372466e1d 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -26,6 +26,7 @@ > #include <asm/hvm/support.h> > #include <asm/iocap.h> > #include <asm/vm_event.h> > +#include <asm/debugreg.h> > > struct hvmemul_cache > { > @@ -2673,7 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt, > } > > if ( hvmemul_ctxt->ctxt.retire.singlestep ) > - hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); > + hvm_inject_debug_exception(DR_STEP); > > new_intr_shadow = hvmemul_ctxt->intr_shadow; > > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c > index d5e8cb0722ca..f25d6a53f092 100644 > --- a/xen/arch/x86/hvm/svm/svm.c > +++ b/xen/arch/x86/hvm/svm/svm.c > @@ -96,7 +96,7 @@ void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len) > curr->arch.hvm.svm.vmcb->int_stat.intr_shadow = 0; > > if ( regs->eflags & X86_EFLAGS_TF ) > - hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); > + hvm_inject_debug_exception(DR_STEP); > } > > static void cf_check svm_cpu_down(void) > @@ -1328,10 +1328,10 @@ static void cf_check svm_inject_event(const struct x86_event *event) > switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) ) > { > case X86_EXC_DB: > - if ( regs->eflags & X86_EFLAGS_TF ) > + if ( event->extra ) > { > __restore_debug_registers(vmcb, curr); > - vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP); > + vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | event->extra); > } > /* fall through */ > case X86_EXC_BP: > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c > index 8823ca13e55d..1795b9479cf9 100644 > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -2022,10 +2022,10 @@ static void cf_check vmx_inject_event(const struct x86_event *event) > switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) ) > { > case X86_EXC_DB: > - if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF ) > + if ( event->extra ) > { > __restore_debug_registers(curr); > - write_debugreg(6, read_debugreg(6) | DR_STEP); > + write_debugreg(6, read_debugreg(6) | event->extra); > } > if ( !nestedhvm_vcpu_in_guestmode(curr) || > !nvmx_intercepts_exception(curr, X86_EXC_DB, _event.error_code) ) > @@ -3068,7 +3068,7 @@ void update_guest_eip(void) > } > > if ( regs->eflags & X86_EFLAGS_TF ) > - hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); > + hvm_inject_debug_exception(DR_STEP); > } > > static void cf_check vmx_fpu_dirty_intercept(void) > diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h > index f3f6310ab684..6a0b9e3ff01e 100644 > --- a/xen/arch/x86/include/asm/hvm/hvm.h > +++ b/xen/arch/x86/include/asm/hvm/hvm.h > @@ -538,6 +538,18 @@ static inline void hvm_inject_page_fault(int errcode, unsigned long cr2) > hvm_inject_event(&event); > } > > +static inline void hvm_inject_debug_exception(unsigned long pending_dbg) > +{ > + struct x86_event event = { > + .vector = X86_EXC_DB, > + .type = X86_EVENTTYPE_HW_EXCEPTION, > + .error_code = X86_EVENT_NO_EC, > + .extra = pending_dbg, > + }; > + > + hvm_inject_event(&event); > +} > + > static inline bool hvm_event_pending(const struct vcpu *v) > { > return alternative_call(hvm_funcs.event_pending, v); > diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c > index cf74fdf5dda6..365af5169750 100644 > --- a/xen/arch/x86/mm/shadow/multi.c > +++ b/xen/arch/x86/mm/shadow/multi.c > @@ -24,6 +24,7 @@ > #include <asm/hvm/cacheattr.h> > #include <asm/mtrr.h> > #include <asm/guest_pt.h> > +#include <asm/debugreg.h> > #include <public/sched.h> > #include "private.h" > #include "types.h" > @@ -2788,7 +2789,7 @@ static int cf_check sh_page_fault( > #endif > > if ( emul_ctxt.ctxt.retire.singlestep ) > - hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); > + hvm_inject_debug_exception(DR_STEP); > > #if GUEST_PAGING_LEVELS == 3 /* PAE guest */ > /* > @@ -2829,7 +2830,7 @@ static int cf_check sh_page_fault( > TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED); > > if ( emul_ctxt.ctxt.retire.singlestep ) > - hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); > + hvm_inject_debug_exception(DR_STEP); > > break; /* Don't emulate again if we failed! */ > } > diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h > index bad957f9bcb2..868a64ab20e6 100644 > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -78,7 +78,9 @@ struct x86_event { > uint8_t type; /* X86_EVENTTYPE_* */ > uint8_t insn_len; /* Instruction length */ > int32_t error_code; /* X86_EVENT_NO_EC if n/a */ > - unsigned long extra; /* CR2 if X86_EXC_PF h/w exception */ > + > + /* Type-specific extra data (%cr2 for #PF, pending_dbg for #DB) */ > + unsigned long extra; > }; > > /*
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 9b6e4c8bc61b..5ad372466e1d 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -26,6 +26,7 @@ #include <asm/hvm/support.h> #include <asm/iocap.h> #include <asm/vm_event.h> +#include <asm/debugreg.h> struct hvmemul_cache { @@ -2673,7 +2674,7 @@ static int _hvm_emulate_one(struct hvm_emulate_ctxt *hvmemul_ctxt, } if ( hvmemul_ctxt->ctxt.retire.singlestep ) - hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); + hvm_inject_debug_exception(DR_STEP); new_intr_shadow = hvmemul_ctxt->intr_shadow; diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index d5e8cb0722ca..f25d6a53f092 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -96,7 +96,7 @@ void __update_guest_eip(struct cpu_user_regs *regs, unsigned int inst_len) curr->arch.hvm.svm.vmcb->int_stat.intr_shadow = 0; if ( regs->eflags & X86_EFLAGS_TF ) - hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); + hvm_inject_debug_exception(DR_STEP); } static void cf_check svm_cpu_down(void) @@ -1328,10 +1328,10 @@ static void cf_check svm_inject_event(const struct x86_event *event) switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) ) { case X86_EXC_DB: - if ( regs->eflags & X86_EFLAGS_TF ) + if ( event->extra ) { __restore_debug_registers(vmcb, curr); - vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | DR_STEP); + vmcb_set_dr6(vmcb, vmcb_get_dr6(vmcb) | event->extra); } /* fall through */ case X86_EXC_BP: diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 8823ca13e55d..1795b9479cf9 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2022,10 +2022,10 @@ static void cf_check vmx_inject_event(const struct x86_event *event) switch ( _event.vector | -(_event.type == X86_EVENTTYPE_SW_INTERRUPT) ) { case X86_EXC_DB: - if ( guest_cpu_user_regs()->eflags & X86_EFLAGS_TF ) + if ( event->extra ) { __restore_debug_registers(curr); - write_debugreg(6, read_debugreg(6) | DR_STEP); + write_debugreg(6, read_debugreg(6) | event->extra); } if ( !nestedhvm_vcpu_in_guestmode(curr) || !nvmx_intercepts_exception(curr, X86_EXC_DB, _event.error_code) ) @@ -3068,7 +3068,7 @@ void update_guest_eip(void) } if ( regs->eflags & X86_EFLAGS_TF ) - hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); + hvm_inject_debug_exception(DR_STEP); } static void cf_check vmx_fpu_dirty_intercept(void) diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index f3f6310ab684..6a0b9e3ff01e 100644 --- a/xen/arch/x86/include/asm/hvm/hvm.h +++ b/xen/arch/x86/include/asm/hvm/hvm.h @@ -538,6 +538,18 @@ static inline void hvm_inject_page_fault(int errcode, unsigned long cr2) hvm_inject_event(&event); } +static inline void hvm_inject_debug_exception(unsigned long pending_dbg) +{ + struct x86_event event = { + .vector = X86_EXC_DB, + .type = X86_EVENTTYPE_HW_EXCEPTION, + .error_code = X86_EVENT_NO_EC, + .extra = pending_dbg, + }; + + hvm_inject_event(&event); +} + static inline bool hvm_event_pending(const struct vcpu *v) { return alternative_call(hvm_funcs.event_pending, v); diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c index cf74fdf5dda6..365af5169750 100644 --- a/xen/arch/x86/mm/shadow/multi.c +++ b/xen/arch/x86/mm/shadow/multi.c @@ -24,6 +24,7 @@ #include <asm/hvm/cacheattr.h> #include <asm/mtrr.h> #include <asm/guest_pt.h> +#include <asm/debugreg.h> #include <public/sched.h> #include "private.h" #include "types.h" @@ -2788,7 +2789,7 @@ static int cf_check sh_page_fault( #endif if ( emul_ctxt.ctxt.retire.singlestep ) - hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); + hvm_inject_debug_exception(DR_STEP); #if GUEST_PAGING_LEVELS == 3 /* PAE guest */ /* @@ -2829,7 +2830,7 @@ static int cf_check sh_page_fault( TRACE_SHADOW_PATH_FLAG(TRCE_SFLAG_EMULATION_LAST_FAILED); if ( emul_ctxt.ctxt.retire.singlestep ) - hvm_inject_hw_exception(X86_EXC_DB, X86_EVENT_NO_EC); + hvm_inject_debug_exception(DR_STEP); break; /* Don't emulate again if we failed! */ } diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h index bad957f9bcb2..868a64ab20e6 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.h +++ b/xen/arch/x86/x86_emulate/x86_emulate.h @@ -78,7 +78,9 @@ struct x86_event { uint8_t type; /* X86_EVENTTYPE_* */ uint8_t insn_len; /* Instruction length */ int32_t error_code; /* X86_EVENT_NO_EC if n/a */ - unsigned long extra; /* CR2 if X86_EXC_PF h/w exception */ + + /* Type-specific extra data (%cr2 for #PF, pending_dbg for #DB) */ + unsigned long extra; }; /*
Today, when a HVM (or PVH) guest triggers a hardware breakpoint while EFLAGS.TF is set, Xen incorrectly assumes that this is a single stepping exception and sets DR_STEP in dr6 in addition to DR_TRAP<n>. This causes problems with Linux HW breakpoint handler, which ignores DR_TRAP<n> bits when DR_STEP is set. This prevents user-mode debuggers from recognizing hardware breakpoints if EFLAGS.TF is set. Fix this by not setting DR_STEP in {vmx,svm}_inject_event, unless the emulator explicitly signals the single-stepping mode via the newly added "singlestep" boolean field of struct x86_event. Fixes: 8b831f4189 ("x86: single step after instruction emulation") Signed-off-by: Jinoh Kang <jinoh.kang.kr@gmail.com> --- xen/arch/x86/hvm/emulate.c | 3 ++- xen/arch/x86/hvm/svm/svm.c | 6 +++--- xen/arch/x86/hvm/vmx/vmx.c | 6 +++--- xen/arch/x86/include/asm/hvm/hvm.h | 12 ++++++++++++ xen/arch/x86/mm/shadow/multi.c | 5 +++-- xen/arch/x86/x86_emulate/x86_emulate.h | 4 +++- 6 files changed, 26 insertions(+), 10 deletions(-)