Hi, I agree that your approach seems to be better. I played around with the patch and it solves the problem. I share your concerns regarding STI/MOV SS scenarios but I wasn't able to observe any unexpected behavior in a few different cases. Also I couldn't find any logical reason standing behind "if (fBlockSti || fBlockMovSS)" check. It seems to be an unnecessary constraint which is a direct cause of the bug. I hope you will come to the same conclusions after your tests.
Regards, Konrad 2013/11/12 Ramshankar <[email protected]> > On 11/12/2013 05:13 PM, Ramshankar wrote: > > Hi, > > Thanks for the testcase. I took a look at your patch and the testcase. > > I don't think injecting #DB directly the way you're suggesting is the > right thing to do. The right fix, as far as I can see, would be to setup > the pending debug exceptions VMCS field, that way VMX injects the #DB > taking into account the priority of exceptions. > > The fix would be to simply remove the "if (fBlockSti || fBlockMovSS)" in > hmR0VmxInjectPendingEvent() but I've not yet tested if it works. Once I > test that it works, I'll do the needful. I need to verify that I'm not > screwing up something else in the block-by-STI and block-by-MovSS scenarios. > > Thanks again for the patch! > > > Oh and you'll have to call hmR0VmxSaveGuestRflags() before the "if > (pMixedCtx->eflags.Bits.u1TF)" check in hmR0VmxInjectPendingEvent(). > > > Regards, > Ram. > > > On 11/12/2013 01:44 PM, Konrad Kuźmiński wrote: > > Hi, > > It looks like gmail silently failed to attach the file and didn't even > notify me about this. I reattached my program in this message. It was > compiled with MASM using attached script and tested under Windows XP SP3 > host and guest. > > Here's the bat file I used for compilation (I cannot attach such files): > @REM Make sure this path is correct before attempting to build > @set MASM_PATH=C:\masm32 > @set PATH=%PATH%;%MASM_PATH%\bin > > ml -c -coff -I%MASM_PATH%\include 10947_test.asm > link /ENTRY:start /SUBSYSTEM:CONSOLE /LIBPATH:%MASM_PATH%\lib > 10947_test.obj > > Regards, > Konrad > > > 2013/11/12 Ramshankar <[email protected]> > >> Hi, >> >> You said in the attachment, you have included a testcase, I can't find >> any attachment in your original email. Could you please re-attach it? >> >> Regards, >> Ram. >> >> >> On 11/09/2013 05:01 AM, Ramshankar wrote: >> >>> Hi, >>> >>> Thank you for the patch. I'll take a look at it next week. >>> >>> Regards, >>> Ram. >>> >>> On 08/11/13 22:13, Konrad Kuźmiński wrote: >>> >>>> Hi, >>>> >>>> I've made a fix for this bug https://www.virtualbox.org/ticket/10947. >>>> Mentioned ticket isn't very descriptive so I will try to explain this >>>> issue a little bit more in detail. First of all this problem occurs only >>>> when VT-x is enabled. Basically some instructions don't generate >>>> expected single step exception after they are executed with the trap >>>> flag being set. The behaviour is observed that such instructions are >>>> executed under the control of the guest system but single step exception >>>> is generated after the next instruction. This is a well known bug >>>> amongst malware researchers and malware authors who can easily take >>>> advantage of this fact in order to detect virtualized environment. >>>> >>>> It turns out that the problem lies in the way VirtualBox handles some VM >>>> exits initiated by the execution of certain instructions. Several >>>> instructions can never be executed in VMX non-root operation and those >>>> need to be emulated and skipped within VM exit handlers by adjusting >>>> RIP. Unfortunately the code lacks necessary check for the trap flag >>>> being set, so it doesn't inject expected exception into the guest. >>>> >>>> Here's the fix: >>>> *** src\VBox\VMM\VMMR0\HMVMXR0_original.cpp 2013-11-01 >>>> 18:58:26.000000000 +0100 >>>> --- src\VBox\VMM\VMMR0\HMVMXR0_fixed.cpp 2013-11-08 >>>> 20:24:30.578125000 +0100 >>>> *************** >>>> *** 8166,8181 **** >>>> --- 8166,8190 ---- >>>> DECLINLINE(int) hmR0VmxAdvanceGuestRip(PVMCPU pVCpu, PCPUMCTX >>>> pMixedCtx, PVMXTRANSIENT pVmxTransient) >>>> { >>>> int rc = hmR0VmxReadExitInstrLenVmcs(pVCpu, pVmxTransient); >>>> rc |= hmR0VmxSaveGuestRip(pVCpu, pMixedCtx); >>>> AssertRCReturn(rc, rc); >>>> >>>> pMixedCtx->rip += pVmxTransient->cbInstr; >>>> VMCPU_HMCF_SET(pVCpu, HM_CHANGED_GUEST_RIP); >>>> + >>>> + X86EFLAGS Eflags; >>>> + rc = VMXReadVmcs32(VMX_VMCS_GUEST_RFLAGS, &Eflags.u32); >>>> + AssertRCReturn(rc, rc); >>>> + if (Eflags.Bits.u1TF) >>>> + { >>>> + hmR0VmxSetPendingXcptDB(pVCpu, pMixedCtx); >>>> + } >>>> + >>>> return rc; >>>> } >>>> >>>> This fix ensures correct handling of mentioned condition in all 13 >>>> affected VM exit handlers: VMX_EXIT_CPUID, VMX_EXIT_RDTSC, >>>> VMX_EXIT_RDTSCP, VMX_EXIT_RDPMC, VMX_EXIT_MOV_CRX, VMX_EXIT_MOV_DRX, >>>> VMX_EXIT_MWAIT, VMX_EXIT_MONITOR, VMX_EXIT_RDMSR, VMX_EXIT_WRMSR, >>>> VMX_EXIT_INVD, VMX_EXIT_INVLPG, VMX_EXIT_WBINVD. >>>> >>>> In the attachment I provided a simple program which can be used to test >>>> this condition on 2 representative instructions: CPUID and RDTSC. I >>>> picked those because they don't require CPL = 0. >>>> >>>> I release this patch and test program under MIT license. >>>> >>>> Best regards, >>>> Konrad >>>> >>>> >>>> _______________________________________________ >>>> vbox-dev mailing list >>>> [email protected] >>>> https://www.virtualbox.org/mailman/listinfo/vbox-dev >>>> >>>> >>> _______________________________________________ >>> vbox-dev mailing list >>> [email protected] >>> https://www.virtualbox.org/mailman/listinfo/vbox-dev >>> >> >> > > > > _______________________________________________ > vbox-dev mailing > [email protected]https://www.virtualbox.org/mailman/listinfo/vbox-dev > > >
_______________________________________________ vbox-dev mailing list [email protected] https://www.virtualbox.org/mailman/listinfo/vbox-dev
