[vbox-dev] [Patch] Fix for bug #10947
Ramshankar
ramshankar.venkataraman at oracle.com
Tue Nov 12 16:32:53 GMT 2013
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 <ramshankar.venkataraman at oracle.com
>> <mailto:ramshankar.venkataraman at oracle.com>>
>>
>> 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
>> vbox-dev at virtualbox.org <mailto:vbox-dev at virtualbox.org>
>> https://www.virtualbox.org/mailman/listinfo/vbox-dev
>>
>>
>> _______________________________________________
>> vbox-dev mailing list
>> vbox-dev at virtualbox.org <mailto:vbox-dev at virtualbox.org>
>> https://www.virtualbox.org/mailman/listinfo/vbox-dev
>>
>>
>>
>
>
>
> _______________________________________________
> vbox-dev mailing list
> vbox-dev at virtualbox.org
> https://www.virtualbox.org/mailman/listinfo/vbox-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.virtualbox.org/pipermail/vbox-dev/attachments/20131112/1138b188/attachment.html>
More information about the vbox-dev
mailing list