Re: Possible problem emulating movntq, movss

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

On 08/06/2014 02:05 PM, Andrew Cooper wrote:
> On 06/08/2014 11:47, Andrei LUTAS wrote:
>> Hello there,
>>
>> On 8/6/2014 12:54 PM, Jan Beulich wrote:
>>>>>> On 06.08.14 at 10:57, <rcojocaru@bitdefender.com> wrote:
>>>> We found that our HVM guests froze when trying to emulate movntq
>>>> instructions. The solution seems to be to replace "goto done;" with
>>>> "break;" at line 4191 (when handling "case 0x7f:") in
>>>> xen/arch/x86/x86_emulate/x86_emulate.c. Otherwise the writeback part
>>>> doesn't happen.
>>>>
>>>> If you're happy with the fix I can prepare a patch, otherwise please
>>>> let
>>>> me know if we're missing something.
>>> No, that doesn't look right: There's nothing left to be written back at
>>> that point (registers get updated with the instruction executed via the
>>> on-stack stub, and memory gets written with immediately preceding
>>> ops->write(). So without you being more specific about _what_ you
>>> see going wrong I don't think I can give further advice.
>> Except for maybe the instruction pointer? That doesn't seem to be updated
>> anywhereexcept during the write-back phase (or maybe I'm missing the
>> spot).
>> The problem is that the guest gets stuck with the instruction pointer
>> pointing to the sameinstruction (in our particular case it is
>> "MOVDQU xmm0, xmmword ptr [rdx + rcx - 0x10]"),entering in an infinite
>> loop (EPT violation - emulate), since the IP doesn't seem to be updated.
>>>
>>> Furthermore what you write is kind of inconsistent: For one, opcode
>>> 0x7f is movq/movdq[au] rather than movntdq (admitted they're
>>> being handled by the same code block, but you ought to be rather
>>> precise here). And then the subject of your mail mentions movss, but
>>> the body doesn't at all - is that because you mean the same would
>>> apply to that other similar code block?
>> A quick look reveals that 0x0f 0x2b/0x28/0x29/0x10/0x11 and 0x0f
>> 0xe7/0x6f/0x7f
>> encodings are all affected. While other places may be affected too,
>> these two
>> encoding sets seem to be the only ones where "goto done;" is used
>> unconditionally instead of a "break;" - all otheruses of "goto done;" are
>> made when an error is encountered.
>>
>>>
>>> As to Andrew asking for added tests: movq, movdqu, and vmovdqu
>>> are all being tested with both operation directions (covering one of
>>> the two code blocks in question), and the set of tests for movsd,
>>> movaps, vmovsd, and vmovaps should be sufficient to cover the
>>> other of the two code blocks too.
>>>
>>> Jan
>> Best regards,
>> Andrei.
> 
> It would appear that for some instructions, these movxxx included, the
> test harness does not verify that the instruction pointer has been
> suitably updated.
> 
> It is plausible that this is a real bug and the unit tests are
> erroneously passing.  I would suggest starting by adding instruction
> pointer checks to the test harness first to confirm whether there is a bug.

Quick and dirty test:

655     printf("%-40s", "Testing movdqu %xmm2,(%ecx)...");
656     if ( stack_exec && cpu_has_sse2 )
657     {
658         extern const unsigned char movdqu_to_mem[];
659
660         asm volatile ( "pcmpeqb %%xmm2, %%xmm2\n"
661                        ".pushsection .test, \"a\", @progbits\n"
662                        "movdqu_to_mem: movdqu %%xmm2, (%0)\n"
663                        ".popsection" :: "c" (NULL) );
664
665         memcpy(instr, movdqu_to_mem, 15);
666         memset(res, 0x55, 64);
667         memset(res + 8, 0xff, 16);
668         regs.eip    = (unsigned long)&instr[0];
669         regs.ecx    = (unsigned long)res;
670         rc = x86_emulate(&ctxt, &emulops);
671         if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
672             goto fail;
673
674         if ( regs.eip == (unsigned long)&instr[0] )
675             printf("eip has NOT been updated!\n");
676         else
677             printf("okay\n");
678     }
679     else
680         printf("skipped\n");

Output:

Testing movdqu %xmm2,(%ecx)...          eip has NOT been updated!


Thanks,
Razvan Cojocaru