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

[Jan Kiszka <jan.kiszka@siemens.com>]

Gleb Natapov wrote:
> 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.

Maybe we can even fix it completely, just need to move some code around
and add checks to those few existing exception handlers. Will think
about it.

> 
>>> 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?

Inject, e.g., an NMI over code with TF set. A bit harder is placing a
guest HW breakpoint at the spot the NMI handler returns to.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux