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

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

[Razvan Cojocaru]

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

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

[Andrew Cooper]

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]) );

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

[Jürgen Groß]

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]) );
>

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

[Razvan Cojocaru]

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

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

[Jan Beulich]

>>> 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

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

[Razvan Cojocaru]

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

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

[Jan Beulich]

>>> 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

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

[Razvan Cojocaru]

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

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

[Razvan Cojocaru]

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

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

[Andrew Cooper]

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