Re: [PATCH 2/2] KVM: SVM: Make stepping out of NMI handlers more robust

[Gleb Natapov <gleb@redhat.com>]

On Tue, Feb 16, 2010 at 10:45:15AM +0100, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Tue, Feb 16, 2010 at 10:14:56AM +0100, Jan Kiszka wrote:
> >> Gleb Natapov wrote:
> >>> On Mon, Feb 15, 2010 at 07:17:18PM +0100, Jan Kiszka wrote:
> >>>> As there is no interception on AMD on the end of an NMI handler but only
> >>>> on its iret, we are forced to step out by setting TF in rflags. This can
> >>>> collide with host or guest using single-step mode, and it may leak the
> >>>> flags onto the guest stack if IRET triggers some exception.
> >>> The code is trying to handle the case where debugger used TF flags and we
> >>> want to single step from NMI handler simultaneously. Do you see problem with
> >>> that code? Uf yes may be it sill be much simpler to fix it? TF leakage is real,
> >>> but what problem it may cause? Note that your patch does not solve this problem
> >>> too. See the comment that you've deleted:
> >>> 	/* Something prevents NMI from been injected. Single step over
> >>> 	   possible problem (IRET or exception injection or interrupt
> >>> 	   shadow) */
> >>> So the reason for single step is not necessary IRET, _any_ exception
> >>> is possible at this point.
> >> That is exactly what my code tries to avoid: Exceptions are all (famous
> >> last word) caught, and single-stepping is disabled until that is
> >> resolved. So no more leakage, and only IRET remains as reason here (thus
> >> my deletion).
> >>
> > I don't understand why only IRET remains as a reason here? Code will get
> > there if interrupt shadow is in effect too and then next instruction may
> > generate any exception not only those that IRET generates.
> 
> OK, so the faults raised by the instruction under the interrupt shadow
> can still cause troubles. Guess we have to live with it unless we what
> to trap all exceptions that instructions can raise. Will adjust the comment.
> 
I don't see the point to complicate code significantly to fix it only
partially.

> > 
> > Also you haven't answered what is the problem with current code (except
> > TF leakage) and why TF leakage is so important. BTW are you sure that TF
> > leakage actually happens? I see in Intel SDM:
> > 
> >  The processor clears the TF flag before calling the exception handler.
> 
> Does it clear it _for_ the exception handler or also in rflags pushed on
> the stack?
Have no idea. Looking for relevant info in SDM.

> 
> Besides this, proper #DB forwarding to the guest was missing.
During NMI injection? How to reproduce?

--
			Gleb.