linux-kernel - Re: [PATCH 3/3] KVM: x86: handle singlestep during emulation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20130731104037.GG7484@redhat.com>
Date:	Wed, 31 Jul 2013 13:40:38 +0300
From:	Gleb Natapov <gleb@...hat.com>
To:	Paolo Bonzini <pbonzini@...hat.com>
Cc:	linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH 3/3] KVM: x86: handle singlestep during emulation

On Wed, Jul 31, 2013 at 06:30:31AM -0400, Paolo Bonzini wrote:
> 
> ----- Gleb Natapov <gleb@...hat.com> wrote:
> > On Tue, Jul 30, 2013 at 05:11:36PM +0200, Paolo Bonzini wrote:
> > > This lets debugging work better during emulation of invalid
> > > guest state.
> > > 
> > > This time the check is done after emulation, but before writeback
> > > of the flags; we need to check the flags *before* execution of the
> > > instruction, we cannot check singlestep_rip because the CS base may
> > > have already been modified.
> > > 
> > > Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> > > ---
> > >  arch/x86/kvm/x86.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 1368cf5..9805cfd 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -4971,6 +4971,41 @@ static int kvm_vcpu_check_hw_bp(unsigned long addr, u32 type, u32 dr7,
> > >  	return dr6;
> > >  }
> > >  
> > > +static void kvm_vcpu_check_singlestep(struct kvm_vcpu *vcpu, int *r)
> > > +{
> > > +	struct kvm_run *kvm_run = vcpu->run;
> > > +
> > > +	/*
> > > +	 * Use the "raw" value to see if TF was passed to the processor.
> > > +	 * Note that the new value of the flags has not been saved yet.
> > > +	 *
> > > +	 * This is correct even for TF set by the guest, because "the
> > > +	 * processor will not generate this exception after the instruction
> > > +	 * that sets the TF flag".
> > > +	 */
> > > +	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
> > > +
> > > +	if (unlikely(rflags & X86_EFLAGS_TF)) {
> > > +		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP) {
> > > +			kvm_run->debug.arch.dr6 = DR6_BS | DR6_FIXED_1;
> > > +			kvm_run->debug.arch.pc = vcpu->arch.singlestep_rip;
> > > +			kvm_run->debug.arch.exception = DB_VECTOR;
> > > +			kvm_run->exit_reason = KVM_EXIT_DEBUG;
> > > +			*r = EMULATE_USER_EXIT;
> > > +		} else {
> > > +			vcpu->arch.emulate_ctxt.eflags &= ~X86_EFLAGS_TF;
> > > +			/*
> > > +			 * "Certain debug exceptions may clear bit 0-3.  The
> > > +			 * remaining contents of the DR6 register are never
> > > +			 * cleared by the processor".
> > > +			 */
> > > +			vcpu->arch.dr6 &= ~15;
> > > +			vcpu->arch.dr6 |= DR6_BS;
> > > +			kvm_queue_exception(vcpu, DB_VECTOR);
> > > +		}
> > > +	}
> > > +}
> > > +
> > >  static bool kvm_vcpu_check_breakpoint(struct kvm_vcpu *vcpu, int *r)
> > >  {
> > >  	struct kvm_run *kvm_run = vcpu->run;
> > > @@ -5117,10 +5152,12 @@ restart:
> > >  
> > >  	if (writeback) {
> > >  		toggle_interruptibility(vcpu, ctxt->interruptibility);
> > > -		kvm_set_rflags(vcpu, ctxt->eflags);
> > >  		kvm_make_request(KVM_REQ_EVENT, vcpu);
> > >  		vcpu->arch.emulate_regs_need_sync_to_vcpu = false;
> > >  		kvm_rip_write(vcpu, ctxt->eip);
> > > +		if (r == EMULATE_DONE)
> > Single step will not work for mmio write and pio out, we never return
> > into emulator for those instructions.
> 
> Ok to apply the patch as is and work it out later (I suppose I need to
> check for NULL complete_userspace_io, and if so set my function)?  It
> is already a huge improvement in usability.
> 
This is not worse from what we have now, so lets apply it, but please add
comment here that write case is not handled properly yet to not forget
about it.

complete_userspace_io is not null for write MMIO but the execution
never returns to emulator, so this is not so simple. May be set
vcpu->arch.io_singlestep and check it in complete_userspace_io.

--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/