The instruction "lock nopl (%rax)" should raise an exception. However,
we don't do that since we do not check for lock prefix for nop
instructions. The following patch adds this check and makes the
behavior similar to hardware.

Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 target/i386/translate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/i386/translate.c b/target/i386/translate.c
index 1d1372fb43..76f4ccd3b4 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -7881,6 +7881,9 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
         gen_nop_modrm(env, s, modrm);
         break;
     case 0x119: case 0x11c ... 0x11f: /* nop (multi byte) */
+        if (prefixes & PREFIX_LOCK) {
+            goto illegal_op;
+        }
         modrm = cpu_ldub_code(env, s->pc++);
         gen_nop_modrm(env, s, modrm);
         break;
-- 
2.13.0

On 05/13/2017 08:58 AM, Pranith Kumar wrote:
> The instruction "lock nopl (%rax)" should raise an exception. However,
> we don't do that since we do not check for lock prefix for nop
> instructions. The following patch adds this check and makes the
> behavior similar to hardware.
> 
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>   target/i386/translate.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/target/i386/translate.c b/target/i386/translate.c
> index 1d1372fb43..76f4ccd3b4 100644
> --- a/target/i386/translate.c
> +++ b/target/i386/translate.c
> @@ -7881,6 +7881,9 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>           gen_nop_modrm(env, s, modrm);
>           break;
>       case 0x119: case 0x11c ... 0x11f: /* nop (multi byte) */
> +        if (prefixes & PREFIX_LOCK) {
> +            goto illegal_op;
> +        }
>           modrm = cpu_ldub_code(env, s->pc++);
>           gen_nop_modrm(env, s, modrm);
>           break;
> 
Surely you'd also want to make this change for 0x11a and 0x11b.  Which would 
also simplify that code a bit.

That said, there's *lots* of missing LOCK prefix checks.  What brings this one 
in particular to your attention?


r~

On Sun, May 14, 2017 at 5:12 PM, Richard Henderson <rth@twiddle.net> wrote:
>>
> Surely you'd also want to make this change for 0x11a and 0x11b.  Which would
> also simplify that code a bit.
>
> That said, there's *lots* of missing LOCK prefix checks.  What brings this
> one in particular to your attention?
>

The motivation for this change is here:
https://github.com/aquynh/capstone/issues/915

Apparently LLVM generates it in certain scenarios when padding with
multi-byte nop (it shouldn't).

From what I understand, a proper instruction like "lock; <valid inst>"
is converted to "lock; multi-byte nop; <valid inst>" due to code
alignment.

There were bugs reported regarding this:
https://bugs.chromium.org/p/nativeclient/issues/detail?id=3929

I am not sure we want to fix this, but I thought it would be easy
enough to cover this case.

Thanks,
-- 
Pranith