[PATCH 1/2] x86/emul: Correct the return value handling of VMFUNC

[PATCH 1/2] x86/emul: Correct the return value handling of VMFUNC

[Andrew Cooper]

The bracketing of x86_emulate() calling the ops->vmfunc() hook is wrong with
respect to the assignment to rc, which can trip the new assertions in
x86_emulate_wrapper().

The hvmemul_vmfunc() hook should only raise #UD if X86EMUL_EXCEPTION is
returned.  This is only a latent bug at the moment.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
---
 xen/arch/x86/hvm/emulate.c             | 2 +-
 xen/arch/x86/x86_emulate/x86_emulate.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 24754d3..aa1b716 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1653,7 +1653,7 @@ static int hvmemul_vmfunc(
     if ( !hvm_funcs.altp2m_vcpu_emulate_vmfunc )
         return X86EMUL_UNHANDLEABLE;
     rc = hvm_funcs.altp2m_vcpu_emulate_vmfunc(ctxt->regs);
-    if ( rc != X86EMUL_OKAY )
+    if ( rc == X86EMUL_EXCEPTION )
         x86_emul_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt);
 
     return rc;
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index b6d7358..3076c0c 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -4463,7 +4463,7 @@ x86_emulate(
             generate_exception_if(lock_prefix | rep_prefix() | (vex.pfx == vex_66),
                                   EXC_UD);
             fail_if(!ops->vmfunc);
-            if ( (rc = ops->vmfunc(ctxt) != X86EMUL_OKAY) )
+            if ( (rc = ops->vmfunc(ctxt)) != X86EMUL_OKAY )
                 goto done;
             goto no_writeback;
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/2] x86/emul: Pass shadow register state to the vmfunc() hook

[Andrew Cooper]

vmfunc can in principle modify register state, so should operate on the shadow
register state rather than the starting state of emulation.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Paul Durrant <paul.durrant@citrix.com>
---
 xen/arch/x86/hvm/emulate.c             | 3 ++-
 xen/arch/x86/x86_emulate/x86_emulate.c | 2 +-
 xen/arch/x86/x86_emulate/x86_emulate.h | 1 +
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index aa1b716..fae666a 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1646,13 +1646,14 @@ static int hvmemul_invlpg(
 }
 
 static int hvmemul_vmfunc(
+    struct cpu_user_regs *regs,
     struct x86_emulate_ctxt *ctxt)
 {
     int rc;
 
     if ( !hvm_funcs.altp2m_vcpu_emulate_vmfunc )
         return X86EMUL_UNHANDLEABLE;
-    rc = hvm_funcs.altp2m_vcpu_emulate_vmfunc(ctxt->regs);
+    rc = hvm_funcs.altp2m_vcpu_emulate_vmfunc(regs);
     if ( rc == X86EMUL_EXCEPTION )
         x86_emul_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt);
 
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 3076c0c..c9ffc56 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -4463,7 +4463,7 @@ x86_emulate(
             generate_exception_if(lock_prefix | rep_prefix() | (vex.pfx == vex_66),
                                   EXC_UD);
             fail_if(!ops->vmfunc);
-            if ( (rc = ops->vmfunc(ctxt)) != X86EMUL_OKAY )
+            if ( (rc = ops->vmfunc(&_regs, ctxt)) != X86EMUL_OKAY )
                 goto done;
             goto no_writeback;
 
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 75f57ba..d70b534 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -448,6 +448,7 @@ struct x86_emulate_ops
 
     /* vmfunc: Emulate VMFUNC via given set of EAX ECX inputs */
     int (*vmfunc)(
+        struct cpu_user_regs *regs,
         struct x86_emulate_ctxt *ctxt);
 };
 
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/2] x86/emul: Correct the return value handling of VMFUNC

[Jan Beulich]

>>> On 21.12.16 at 17:32, <andrew.cooper3@citrix.com> wrote:
> The bracketing of x86_emulate() calling the ops->vmfunc() hook is wrong with
> respect to the assignment to rc, which can trip the new assertions in
> x86_emulate_wrapper().
> 
> The hvmemul_vmfunc() hook should only raise #UD if X86EMUL_EXCEPTION is
> returned.  This is only a latent bug at the moment.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1653,7 +1653,7 @@ static int hvmemul_vmfunc(
>      if ( !hvm_funcs.altp2m_vcpu_emulate_vmfunc )
>          return X86EMUL_UNHANDLEABLE;
>      rc = hvm_funcs.altp2m_vcpu_emulate_vmfunc(ctxt->regs);
> -    if ( rc != X86EMUL_OKAY )
> +    if ( rc == X86EMUL_EXCEPTION )
>          x86_emul_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt);

Great - saves me from submitting one of my follow-up patches.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/emul: Pass shadow register state to the vmfunc() hook

[Jan Beulich]

>>> On 21.12.16 at 17:32, <andrew.cooper3@citrix.com> wrote:
> vmfunc can in principle modify register state, so should operate on the shadow
> register state rather than the starting state of emulation.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

While in principle this is fine, I'd rather see the register state
constified for now, to demonstrate it is not being modified. I'll
submit my two remaining follow-up patches in a minute, and
we can then discuss which of the two to take.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] x86/emul: Pass shadow register state to the vmfunc() hook

[Andrew Cooper]

On 22/12/16 08:27, Jan Beulich wrote:
>>>> On 21.12.16 at 17:32, <andrew.cooper3@citrix.com> wrote:
>> vmfunc can in principle modify register state, so should operate on the shadow
>> register state rather than the starting state of emulation.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> While in principle this is fine, I'd rather see the register state
> constified for now, to demonstrate it is not being modified. I'll
> submit my two remaining follow-up patches in a minute, and
> we can then discuss which of the two to take.

The question here is how likely it is that new functionality for VMFUNC 
will be defined, which starts mutating the values.

I am not aware of anything new, so lets go with the const version for 
now (as it is one fewer parameters).  If this changes in the future, we 
can easily switch back to passing the shadow register block.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel