Re: [PATCH] tools/tests: Add EIP check to test_x86_emulator.c

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/08/14 10:17, Razvan Cojocaru wrote:
> On 08/07/2014 11:18 AM, Andrew Cooper wrote:
>> On 07/08/2014 09:16, Razvan Cojocaru wrote:
>>> The test now also checks that EIP was modified after emulating
>>> instructions after (and including) the "movq %mm3,(%ecx)..."
>>> code block.
>>>
>>> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> These checks do check that the instruction pointer has changed, which
>> catches your problem, but doesn't check that it has changed correctly. 
>> You need to work out exactly how long the instruction is a verify that
>> eip points to first byte of the next instruction.
> Code like this:
>
> 602     printf("%-40s", "Testing movq %mm3,(%ecx)...");
> 603     if ( stack_exec && cpu_has_mmx )
> 604     {
> 605         extern const unsigned char movq_to_mem[];
> 606
> 607         asm volatile ( "pcmpeqb %%mm3, %%mm3\n"
> 608                        ".pushsection .test, \"a\", @progbits\n"
> 609                        "movq_to_mem: movq %%mm3, (%0)\n"
> 610                        ".popsection" :: "c" (NULL) );
> 611
> 612         memcpy(instr, movq_to_mem, 15);
> 613         memset(res, 0x33, 64);
> 614         memset(res + 8, 0xff, 8);
> 615         regs.eip    = (unsigned long)&instr[0];
> 616         regs.ecx    = (unsigned long)res;
> 617         rc = x86_emulate(&ctxt, &emulops);
> 618         if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
> 619              (regs.eip == (unsigned long)&instr[0]) )
> 620             goto fail;
> 621         printf("okay\n");
> 622     }
> 623     else
> 624         printf("skipped\n");
>
> probably shows how useful a generic Xen way of determining instruction
> length would be. Where for previous cases it was clear what the length
> of the instruction was (since it was being built by assigning each byte
> of the instruction buffer explicitly), it's not trivial to determine the
> length of a buffer containing a "compiled" instruction and check that
> EIP has advanced by the correct ammount.

I suggest something like:

extern const uint8_t movq_to_mem_start[], movq_to_mem_end[];

...

".pushsection .test, \"a\", @progbits\n"
"movq_to_mem_start: movq %%mm3, (%0)\n"
"movq_to_mem_end:"
".popsection"

At this point, the difference between end and start is however many
bytes the assembler decided to use for the movq instruction, and is
perhaps safer than trying to work it out by hand (especially for the VEX
instructions where there are several valid encodings)

~Andrew