* [PATCH] tools/tests: Add EIP check to test_x86_emulator.c
@ 2014-08-07 8:16 Razvan Cojocaru
2014-08-07 8:18 ` Andrew Cooper
2014-08-07 8:23 ` Jürgen Groß
0 siblings, 2 replies; 10+ messages in thread
From: Razvan Cojocaru @ 2014-08-07 8:16 UTC (permalink / raw)
To: xen-devel
Cc: ian.campbell, Razvan Cojocaru, stefano.stabellini, andrew.cooper3,
ian.jackson, JBeulich
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>
---
tools/tests/x86_emulator/test_x86_emulator.c | 30 +++++++++++++++++---------
1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index 0a00d5a..dd4e986 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -615,7 +615,8 @@ int main(int argc, char **argv)
regs.eip = (unsigned long)&instr[0];
regs.ecx = (unsigned long)res;
rc = x86_emulate(&ctxt, &emulops);
- if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
+ if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
+ (regs.eip == (unsigned long)&instr[0]) )
goto fail;
printf("okay\n");
}
@@ -637,7 +638,8 @@ int main(int argc, char **argv)
regs.ecx = 0;
regs.edx = (unsigned long)res;
rc = x86_emulate(&ctxt, &emulops);
- if ( rc != X86EMUL_OKAY )
+ if ( rc != X86EMUL_OKAY ||
+ (regs.eip == (unsigned long)&instr[0]) )
goto fail;
asm ( "pcmpeqb %%mm3, %%mm3\n\t"
"pcmpeqb %%mm5, %%mm3\n\t"
@@ -665,7 +667,8 @@ int main(int argc, char **argv)
regs.eip = (unsigned long)&instr[0];
regs.ecx = (unsigned long)res;
rc = x86_emulate(&ctxt, &emulops);
- if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
+ if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
+ (regs.eip == (unsigned long)&instr[0]) )
goto fail;
printf("okay\n");
}
@@ -687,7 +690,8 @@ int main(int argc, char **argv)
regs.ecx = 0;
regs.edx = (unsigned long)res;
rc = x86_emulate(&ctxt, &emulops);
- if ( rc != X86EMUL_OKAY )
+ if ( rc != X86EMUL_OKAY ||
+ (regs.eip == (unsigned long)&instr[0]) )
goto fail;
asm ( "pcmpeqb %%xmm2, %%xmm2\n\t"
"pcmpeqb %%xmm4, %%xmm2\n\t"
@@ -716,7 +720,8 @@ int main(int argc, char **argv)
regs.eip = (unsigned long)&instr[0];
regs.ecx = (unsigned long)res;
rc = x86_emulate(&ctxt, &emulops);
- if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 16, 64) )
+ if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 16, 64) ||
+ (regs.eip == (unsigned long)&instr[0]) )
goto fail;
printf("okay\n");
}
@@ -744,7 +749,8 @@ int main(int argc, char **argv)
regs.ecx = 0;
regs.edx = (unsigned long)res;
rc = x86_emulate(&ctxt, &emulops);
- if ( rc != X86EMUL_OKAY )
+ if ( rc != X86EMUL_OKAY ||
+ (regs.eip == (unsigned long)&instr[0]) )
goto fail;
#if 0 /* Don't use AVX2 instructions for now */
asm ( "vpcmpeqb %%ymm2, %%ymm2, %%ymm2\n\t"
@@ -784,7 +790,8 @@ int main(int argc, char **argv)
regs.ecx = (unsigned long)(res + 2);
regs.edx = 0;
rc = x86_emulate(&ctxt, &emulops);
- if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
+ if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
+ (regs.eip == (unsigned long)&instr[0]) )
goto fail;
printf("okay\n");
}
@@ -809,7 +816,8 @@ int main(int argc, char **argv)
regs.ecx = 0;
regs.edx = (unsigned long)res;
rc = x86_emulate(&ctxt, &emulops);
- if ( rc != X86EMUL_OKAY )
+ if ( rc != X86EMUL_OKAY ||
+ (regs.eip == (unsigned long)&instr[0]) )
goto fail;
asm ( "cmpeqps %1, %%xmm7\n\t"
"movmskps %%xmm7, %0" : "=r" (rc) : "m" (res[8]) );
@@ -837,7 +845,8 @@ int main(int argc, char **argv)
regs.ecx = (unsigned long)(res + 2);
regs.edx = 0;
rc = x86_emulate(&ctxt, &emulops);
- if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
+ if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
+ (regs.eip == (unsigned long)&instr[0]) )
goto fail;
printf("okay\n");
}
@@ -862,7 +871,8 @@ int main(int argc, char **argv)
regs.ecx = 0;
regs.edx = (unsigned long)res;
rc = x86_emulate(&ctxt, &emulops);
- if ( rc != X86EMUL_OKAY )
+ if ( rc != X86EMUL_OKAY ||
+ (regs.eip == (unsigned long)&instr[0]) )
goto fail;
asm ( "vcmpeqps %1, %%ymm7, %%ymm0\n\t"
"vmovmskps %%ymm0, %0" : "=r" (rc) : "m" (res[8]) );
--
1.7.9.5
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] tools/tests: Add EIP check to test_x86_emulator.c
2014-08-07 8:16 [PATCH] tools/tests: Add EIP check to test_x86_emulator.c Razvan Cojocaru
@ 2014-08-07 8:18 ` Andrew Cooper
2014-08-07 8:32 ` Jan Beulich
2014-08-07 9:17 ` Razvan Cojocaru
2014-08-07 8:23 ` Jürgen Groß
1 sibling, 2 replies; 10+ messages in thread
From: Andrew Cooper @ 2014-08-07 8:18 UTC (permalink / raw)
To: Razvan Cojocaru, xen-devel
Cc: ian.jackson, ian.campbell, JBeulich, stefano.stabellini
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.
~Andrew
> ---
> tools/tests/x86_emulator/test_x86_emulator.c | 30 +++++++++++++++++---------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
> index 0a00d5a..dd4e986 100644
> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> @@ -615,7 +615,8 @@ int main(int argc, char **argv)
> regs.eip = (unsigned long)&instr[0];
> regs.ecx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> printf("okay\n");
> }
> @@ -637,7 +638,8 @@ int main(int argc, char **argv)
> regs.ecx = 0;
> regs.edx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( rc != X86EMUL_OKAY )
> + if ( rc != X86EMUL_OKAY ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> asm ( "pcmpeqb %%mm3, %%mm3\n\t"
> "pcmpeqb %%mm5, %%mm3\n\t"
> @@ -665,7 +667,8 @@ int main(int argc, char **argv)
> regs.eip = (unsigned long)&instr[0];
> regs.ecx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> printf("okay\n");
> }
> @@ -687,7 +690,8 @@ int main(int argc, char **argv)
> regs.ecx = 0;
> regs.edx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( rc != X86EMUL_OKAY )
> + if ( rc != X86EMUL_OKAY ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> asm ( "pcmpeqb %%xmm2, %%xmm2\n\t"
> "pcmpeqb %%xmm4, %%xmm2\n\t"
> @@ -716,7 +720,8 @@ int main(int argc, char **argv)
> regs.eip = (unsigned long)&instr[0];
> regs.ecx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 16, 64) )
> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 16, 64) ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> printf("okay\n");
> }
> @@ -744,7 +749,8 @@ int main(int argc, char **argv)
> regs.ecx = 0;
> regs.edx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( rc != X86EMUL_OKAY )
> + if ( rc != X86EMUL_OKAY ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> #if 0 /* Don't use AVX2 instructions for now */
> asm ( "vpcmpeqb %%ymm2, %%ymm2, %%ymm2\n\t"
> @@ -784,7 +790,8 @@ int main(int argc, char **argv)
> regs.ecx = (unsigned long)(res + 2);
> regs.edx = 0;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> printf("okay\n");
> }
> @@ -809,7 +816,8 @@ int main(int argc, char **argv)
> regs.ecx = 0;
> regs.edx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( rc != X86EMUL_OKAY )
> + if ( rc != X86EMUL_OKAY ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> asm ( "cmpeqps %1, %%xmm7\n\t"
> "movmskps %%xmm7, %0" : "=r" (rc) : "m" (res[8]) );
> @@ -837,7 +845,8 @@ int main(int argc, char **argv)
> regs.ecx = (unsigned long)(res + 2);
> regs.edx = 0;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> printf("okay\n");
> }
> @@ -862,7 +871,8 @@ int main(int argc, char **argv)
> regs.ecx = 0;
> regs.edx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( rc != X86EMUL_OKAY )
> + if ( rc != X86EMUL_OKAY ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> asm ( "vcmpeqps %1, %%ymm7, %%ymm0\n\t"
> "vmovmskps %%ymm0, %0" : "=r" (rc) : "m" (res[8]) );
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tools/tests: Add EIP check to test_x86_emulator.c
2014-08-07 8:18 ` Andrew Cooper
@ 2014-08-07 8:32 ` Jan Beulich
2014-08-07 8:35 ` Razvan Cojocaru
2014-08-07 9:17 ` Razvan Cojocaru
1 sibling, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2014-08-07 8:32 UTC (permalink / raw)
To: Razvan Cojocaru, Andrew Cooper
Cc: xen-devel, ian.jackson, ian.campbell, stefano.stabellini
>>> On 07.08.14 at 10:18, <andrew.cooper3@citrix.com> 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.
And apart from that this should be done for all tests, not just the
MMX/SSE/AVX ones.
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tools/tests: Add EIP check to test_x86_emulator.c
2014-08-07 8:32 ` Jan Beulich
@ 2014-08-07 8:35 ` Razvan Cojocaru
2014-08-07 8:45 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Razvan Cojocaru @ 2014-08-07 8:35 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: stefano.stabellini, ian.jackson, ian.campbell, xen-devel
On 08/07/2014 11:32 AM, Jan Beulich wrote:
>>>> On 07.08.14 at 10:18, <andrew.cooper3@citrix.com> 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.
>
> And apart from that this should be done for all tests, not just the
> MMX/SSE/AVX ones.
The other tests already do this, but there it's simpler to check. For
example:
218 printf("%-40s", "Testing addl %%ecx,%%eax...");
219 instr[0] = 0x01; instr[1] = 0xc8;
220 regs.eflags = 0x200;
221 regs.eip = (unsigned long)&instr[0];
222 regs.ecx = 0x12345678;
223 regs.eax = 0x7FFFFFFF;
224 rc = x86_emulate(&ctxt, &emulops);
225 if ( (rc != X86EMUL_OKAY) ||
226 (regs.ecx != 0x12345678) ||
227 (regs.eax != 0x92345677) ||
228 (regs.eflags != 0xa94) ||
229 (regs.eip != (unsigned long)&instr[2]) )
230 goto fail;
231 printf("okay\n");
Thanks,
Razvan Cojocaru
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tools/tests: Add EIP check to test_x86_emulator.c
2014-08-07 8:35 ` Razvan Cojocaru
@ 2014-08-07 8:45 ` Jan Beulich
2014-08-07 8:53 ` Razvan Cojocaru
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2014-08-07 8:45 UTC (permalink / raw)
To: Razvan Cojocaru
Cc: Andrew Cooper, xen-devel, ian.jackson, ian.campbell,
stefano.stabellini
>>> On 07.08.14 at 10:35, <rcojocaru@bitdefender.com> wrote:
> On 08/07/2014 11:32 AM, Jan Beulich wrote:
>>>>> On 07.08.14 at 10:18, <andrew.cooper3@citrix.com> 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.
>>
>> And apart from that this should be done for all tests, not just the
>> MMX/SSE/AVX ones.
>
> The other tests already do this, but there it's simpler to check.
Ah, my bad then (both in not having checked the code before
replying, and in not having added these checks back when I
added those tests) - I'm sorry.
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tools/tests: Add EIP check to test_x86_emulator.c
2014-08-07 8:45 ` Jan Beulich
@ 2014-08-07 8:53 ` Razvan Cojocaru
0 siblings, 0 replies; 10+ messages in thread
From: Razvan Cojocaru @ 2014-08-07 8:53 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, stefano.stabellini, ian.jackson, ian.campbell,
xen-devel
On 08/07/2014 11:45 AM, Jan Beulich wrote:
>>>> On 07.08.14 at 10:35, <rcojocaru@bitdefender.com> wrote:
>> On 08/07/2014 11:32 AM, Jan Beulich wrote:
>>>>>> On 07.08.14 at 10:18, <andrew.cooper3@citrix.com> 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.
>>>
>>> And apart from that this should be done for all tests, not just the
>>> MMX/SSE/AVX ones.
>>
>> The other tests already do this, but there it's simpler to check.
>
> Ah, my bad then (both in not having checked the code before
> replying, and in not having added these checks back when I
> added those tests) - I'm sorry.
Absolutely no problem at all, we're all friends here.
Thanks,
Razvan Cojocaru
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tools/tests: Add EIP check to test_x86_emulator.c
2014-08-07 8:18 ` Andrew Cooper
2014-08-07 8:32 ` Jan Beulich
@ 2014-08-07 9:17 ` Razvan Cojocaru
2014-08-07 9:35 ` Andrew Cooper
1 sibling, 1 reply; 10+ messages in thread
From: Razvan Cojocaru @ 2014-08-07 9:17 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: ian.jackson, ian.campbell, JBeulich, stefano.stabellini
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.
Thanks,
Razvan Cojocaru
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tools/tests: Add EIP check to test_x86_emulator.c
2014-08-07 9:17 ` Razvan Cojocaru
@ 2014-08-07 9:35 ` Andrew Cooper
0 siblings, 0 replies; 10+ messages in thread
From: Andrew Cooper @ 2014-08-07 9:35 UTC (permalink / raw)
To: Razvan Cojocaru, xen-devel
Cc: ian.jackson, ian.campbell, JBeulich, stefano.stabellini
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] tools/tests: Add EIP check to test_x86_emulator.c
2014-08-07 8:16 [PATCH] tools/tests: Add EIP check to test_x86_emulator.c Razvan Cojocaru
2014-08-07 8:18 ` Andrew Cooper
@ 2014-08-07 8:23 ` Jürgen Groß
2014-08-07 8:27 ` Razvan Cojocaru
1 sibling, 1 reply; 10+ messages in thread
From: Jürgen Groß @ 2014-08-07 8:23 UTC (permalink / raw)
To: Razvan Cojocaru, xen-devel
Cc: andrew.cooper3, ian.jackson, ian.campbell, JBeulich,
stefano.stabellini
On 08/07/2014 10:16 AM, 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>
> ---
> tools/tests/x86_emulator/test_x86_emulator.c | 30 +++++++++++++++++---------
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
> index 0a00d5a..dd4e986 100644
> --- a/tools/tests/x86_emulator/test_x86_emulator.c
> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
> @@ -615,7 +615,8 @@ int main(int argc, char **argv)
> regs.eip = (unsigned long)&instr[0];
> regs.ecx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
> + (regs.eip == (unsigned long)&instr[0]) )
Is really any modification okay? I think the test should check for
the correct EIP, not just for one of 2^64 - 1 incorrect values.
> goto fail;
> printf("okay\n");
> }
> @@ -637,7 +638,8 @@ int main(int argc, char **argv)
> regs.ecx = 0;
> regs.edx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( rc != X86EMUL_OKAY )
> + if ( rc != X86EMUL_OKAY ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> asm ( "pcmpeqb %%mm3, %%mm3\n\t"
> "pcmpeqb %%mm5, %%mm3\n\t"
> @@ -665,7 +667,8 @@ int main(int argc, char **argv)
> regs.eip = (unsigned long)&instr[0];
> regs.ecx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> printf("okay\n");
> }
> @@ -687,7 +690,8 @@ int main(int argc, char **argv)
> regs.ecx = 0;
> regs.edx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( rc != X86EMUL_OKAY )
> + if ( rc != X86EMUL_OKAY ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> asm ( "pcmpeqb %%xmm2, %%xmm2\n\t"
> "pcmpeqb %%xmm4, %%xmm2\n\t"
> @@ -716,7 +720,8 @@ int main(int argc, char **argv)
> regs.eip = (unsigned long)&instr[0];
> regs.ecx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 16, 64) )
> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 16, 64) ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> printf("okay\n");
> }
> @@ -744,7 +749,8 @@ int main(int argc, char **argv)
> regs.ecx = 0;
> regs.edx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( rc != X86EMUL_OKAY )
> + if ( rc != X86EMUL_OKAY ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> #if 0 /* Don't use AVX2 instructions for now */
> asm ( "vpcmpeqb %%ymm2, %%ymm2, %%ymm2\n\t"
> @@ -784,7 +790,8 @@ int main(int argc, char **argv)
> regs.ecx = (unsigned long)(res + 2);
> regs.edx = 0;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> printf("okay\n");
> }
> @@ -809,7 +816,8 @@ int main(int argc, char **argv)
> regs.ecx = 0;
> regs.edx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( rc != X86EMUL_OKAY )
> + if ( rc != X86EMUL_OKAY ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> asm ( "cmpeqps %1, %%xmm7\n\t"
> "movmskps %%xmm7, %0" : "=r" (rc) : "m" (res[8]) );
> @@ -837,7 +845,8 @@ int main(int argc, char **argv)
> regs.ecx = (unsigned long)(res + 2);
> regs.edx = 0;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> printf("okay\n");
> }
> @@ -862,7 +871,8 @@ int main(int argc, char **argv)
> regs.ecx = 0;
> regs.edx = (unsigned long)res;
> rc = x86_emulate(&ctxt, &emulops);
> - if ( rc != X86EMUL_OKAY )
> + if ( rc != X86EMUL_OKAY ||
> + (regs.eip == (unsigned long)&instr[0]) )
> goto fail;
> asm ( "vcmpeqps %1, %%ymm7, %%ymm0\n\t"
> "vmovmskps %%ymm0, %0" : "=r" (rc) : "m" (res[8]) );
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] tools/tests: Add EIP check to test_x86_emulator.c
2014-08-07 8:23 ` Jürgen Groß
@ 2014-08-07 8:27 ` Razvan Cojocaru
0 siblings, 0 replies; 10+ messages in thread
From: Razvan Cojocaru @ 2014-08-07 8:27 UTC (permalink / raw)
To: Jürgen Groß, xen-devel
Cc: andrew.cooper3, ian.jackson, ian.campbell, JBeulich,
stefano.stabellini
On 08/07/2014 11:23 AM, Jürgen Groß wrote:
> On 08/07/2014 10:16 AM, 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>
>> ---
>> tools/tests/x86_emulator/test_x86_emulator.c | 30
>> +++++++++++++++++---------
>> 1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/tools/tests/x86_emulator/test_x86_emulator.c
>> b/tools/tests/x86_emulator/test_x86_emulator.c
>> index 0a00d5a..dd4e986 100644
>> --- a/tools/tests/x86_emulator/test_x86_emulator.c
>> +++ b/tools/tests/x86_emulator/test_x86_emulator.c
>> @@ -615,7 +615,8 @@ int main(int argc, char **argv)
>> regs.eip = (unsigned long)&instr[0];
>> regs.ecx = (unsigned long)res;
>> rc = x86_emulate(&ctxt, &emulops);
>> - if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) )
>> + if ( (rc != X86EMUL_OKAY) || memcmp(res, res + 8, 32) ||
>> + (regs.eip == (unsigned long)&instr[0]) )
>
> Is really any modification okay? I think the test should check for
> the correct EIP, not just for one of 2^64 - 1 incorrect values.
While you are, of course, perfectly right that not any modification is
okay, this does at least catch the bigger issue of EIP not being updated
at all. I'll look into it.
Thanks,
Razvan Cojocaru
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-08-07 9:35 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-07 8:16 [PATCH] tools/tests: Add EIP check to test_x86_emulator.c Razvan Cojocaru
2014-08-07 8:18 ` Andrew Cooper
2014-08-07 8:32 ` Jan Beulich
2014-08-07 8:35 ` Razvan Cojocaru
2014-08-07 8:45 ` Jan Beulich
2014-08-07 8:53 ` Razvan Cojocaru
2014-08-07 9:17 ` Razvan Cojocaru
2014-08-07 9:35 ` Andrew Cooper
2014-08-07 8:23 ` Jürgen Groß
2014-08-07 8:27 ` Razvan Cojocaru
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).