wrong placement of RIP-relative address computing

wrong placement of RIP-relative address computing

[Li, Xin]

Avi,
I met a wrong address from instruction emulation, and it corrupted
guest page table silently.  seems it's caused by eip which has not
reached next instruction.  In my case, eip was pointing to immediate
data of current instruction.

I used below patch to fix it, but I didn't further check other logic.  Can
you please take a look?

Thanks!
-Xin


diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 6df88c7..e269d20 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -3547,9 +3547,6 @@ done_prefixes:
        if (memop.type == OP_MEM && c->ad_bytes != 8)
                memop.addr.mem.ea = (u32)memop.addr.mem.ea;

-       if (memop.type == OP_MEM && c->rip_relative)
-               memop.addr.mem.ea += c->eip;
-
        /*
         * Decode and fetch the source operand: register, memory
         * or immediate.
@@ -3704,6 +3701,9 @@ done_prefixes:
        }

 done:
+       if (memop.type == OP_MEM && c->rip_relative)
+               memop.addr.mem.ea += c->eip;
+
        return (rc == X86EMUL_UNHANDLEABLE) ? EMULATION_FAILED : EMULATION_OK;
 }

RE: wrong placement of RIP-relative address computing

[Li, Xin]

> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 6df88c7..e269d20 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -3547,9 +3547,6 @@ done_prefixes:
>         if (memop.type == OP_MEM && c->ad_bytes != 8)
>                 memop.addr.mem.ea = (u32)memop.addr.mem.ea;
> 
> -       if (memop.type == OP_MEM && c->rip_relative)
> -               memop.addr.mem.ea += c->eip;
> -
>         /*
>          * Decode and fetch the source operand: register, memory
>          * or immediate.
> @@ -3704,6 +3701,9 @@ done_prefixes:
>         }
> 
>  done:

It's not correct to move here, I saw some other problem.

> +       if (memop.type == OP_MEM && c->rip_relative)
> +               memop.addr.mem.ea += c->eip;
> +
>         return (rc == X86EMUL_UNHANDLEABLE) ? EMULATION_FAILED :
> EMULATION_OK;

Re: wrong placement of RIP-relative address computing

[Avi Kivity]

On 06/16/2011 02:00 PM, Li, Xin wrote:
> >  diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> >  index 6df88c7..e269d20 100644
> >  --- a/arch/x86/kvm/emulate.c
> >  +++ b/arch/x86/kvm/emulate.c
> >  @@ -3547,9 +3547,6 @@ done_prefixes:
> >          if (memop.type == OP_MEM&&  c->ad_bytes != 8)
> >                  memop.addr.mem.ea = (u32)memop.addr.mem.ea;
> >
> >  -       if (memop.type == OP_MEM&&  c->rip_relative)
> >  -               memop.addr.mem.ea += c->eip;
> >  -
> >          /*
> >           * Decode and fetch the source operand: register, memory
> >           * or immediate.
> >  @@ -3704,6 +3701,9 @@ done_prefixes:
> >          }
> >
> >   done:
>
> It's not correct to move here, I saw some other problem.
>
> >  +       if (memop.type == OP_MEM&&  c->rip_relative)
> >  +               memop.addr.mem.ea += c->eip;
> >  +
> >          return (rc == X86EMUL_UNHANDLEABLE) ? EMULATION_FAILED :
> >  EMULATION_OK;
>

There's the "return 0" in the previous switch, but otherwise I think 
it's correct.  We didn't take into account the complete instruction length.

-- 
error compiling committee.c: too many arguments to function