[PATCH 1/4] hvm/fep: Allow testing of instructions crossing the -1 -> 0 virtual boundary

[PATCH 1/4] hvm/fep: Allow testing of instructions crossing the -1 -> 0 virtual boundary

[Andrew Cooper]

The Force Emulation Prefix is named to follow its PV counterpart for cpuid or
rdtsc, but isn't really an instruction prefix.  It behaves as a break-out into
Xen, with the purpose of emulating the next instruction in the current state.

It is important to be able to test legal situations which occur in real
hardware, including instruction which cross certain boundaries, and
instructions starting at 0.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/hvm/hvm.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 787f055..596a903 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3981,15 +3981,8 @@ void hvm_ud_intercept(struct cpu_user_regs *regs)
         unsigned long addr;
         char sig[5]; /* ud2; .ascii "xen" */
 
-        /*
-         * Note that in the call below we pass 1 more than the signature
-         * size, to guard against the overall code sequence wrapping between
-         * "prefix" and actual instruction. There's necessarily at least one
-         * actual instruction byte required, so this won't cause failure on
-         * legitimate uses.
-         */
         if ( hvm_virtual_to_linear_addr(x86_seg_cs, cs, regs->eip,
-                                        sizeof(sig) + 1, hvm_access_insn_fetch,
+                                        sizeof(sig), hvm_access_insn_fetch,
                                         (hvm_long_mode_enabled(cur) &&
                                          cs->attr.fields.l) ? 64 :
                                         cs->attr.fields.db ? 32 : 16, &addr) &&
@@ -3999,6 +3992,11 @@ void hvm_ud_intercept(struct cpu_user_regs *regs)
         {
             regs->eip += sizeof(sig);
             regs->eflags &= ~X86_EFLAGS_RF;
+
+            /* Zero the upper 32 bits of %rip if not in long mode. */
+            if ( !(hvm_long_mode_enabled(cur) && cs->attr.fields.l) )
+                regs->eip = regs->_eip;
+
             add_taint(TAINT_HVM_FEP);
         }
     }
-- 
2.1.4


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

[PATCH 2/4] x86/segment: Bounds check accesses to emulation ctxt->seg_reg[]

[Andrew Cooper]

HVM HAP codepaths have space for all segment registers in the seg_reg[]
cache (with x86_seg_none still risking an array overrun), while the shadow
codepaths only have space for the user segments.

Range check the input segment of *_get_seg_reg() against the size of the array
used to cache the results, to avoid overruns in the case that the callers
don't filter their input suitably.

Subsume the is_x86_user_segment(seg) checks from the shadow code, which were
an incomplete attempt at range checking, and are now superceeded.  Make
hvm_get_seg_reg() static, as it is not used outside of shadow/common.c

No functional change, but far easier to reason that no overflow is possible.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/hvm/emulate.c        | 16 ++++++++++++++++
 xen/arch/x86/mm/shadow/common.c   | 27 ++++++++++++++-------------
 xen/arch/x86/mm/shadow/private.h  |  2 --
 xen/include/asm-x86/hvm/emulate.h |  1 +
 4 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index c55ad7b..0eb7a4d 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -535,6 +535,8 @@ static int hvmemul_virtual_to_linear(
     *reps = min_t(unsigned long, *reps, max_reps);
 
     reg = hvmemul_get_seg_reg(seg, hvmemul_ctxt);
+    if ( IS_ERR(reg) )
+        return -PTR_ERR(reg);
 
     if ( (hvmemul_ctxt->ctxt.regs->eflags & X86_EFLAGS_DF) && (*reps > 1) )
     {
@@ -1430,6 +1432,10 @@ static int hvmemul_read_segment(
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
     struct segment_register *sreg = hvmemul_get_seg_reg(seg, hvmemul_ctxt);
+
+    if ( IS_ERR(sreg) )
+         return -PTR_ERR(sreg);
+
     memcpy(reg, sreg, sizeof(struct segment_register));
     return X86EMUL_OKAY;
 }
@@ -1443,6 +1449,9 @@ static int hvmemul_write_segment(
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
     struct segment_register *sreg = hvmemul_get_seg_reg(seg, hvmemul_ctxt);
 
+    if ( IS_ERR(sreg) )
+         return -PTR_ERR(sreg);
+
     memcpy(sreg, reg, sizeof(struct segment_register));
     __set_bit(seg, &hvmemul_ctxt->seg_reg_dirty);
 
@@ -1995,10 +2004,17 @@ void hvm_emulate_writeback(
     }
 }
 
+/*
+ * Callers which pass a known in-range x86_segment can rely on the return
+ * pointer being valid.  Other callers must explicitly check for errors.
+ */
 struct segment_register *hvmemul_get_seg_reg(
     enum x86_segment seg,
     struct hvm_emulate_ctxt *hvmemul_ctxt)
 {
+    if ( seg < 0 || seg >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
+        return ERR_PTR(-X86EMUL_UNHANDLEABLE);
+
     if ( !__test_and_set_bit(seg, &hvmemul_ctxt->seg_reg_accessed) )
         hvm_get_segment_register(current, seg, &hvmemul_ctxt->seg_reg[seg]);
     return &hvmemul_ctxt->seg_reg[seg];
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 7032869..8d6661c 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -123,10 +123,19 @@ __initcall(shadow_audit_key_init);
 /* x86 emulator support for the shadow code
  */
 
-struct segment_register *hvm_get_seg_reg(
+/*
+ * Callers which pass a known in-range x86_segment can rely on the return
+ * pointer being valid.  Other callers must explicitly check for errors.
+ */
+static struct segment_register *hvm_get_seg_reg(
     enum x86_segment seg, struct sh_emulate_ctxt *sh_ctxt)
 {
-    struct segment_register *seg_reg = &sh_ctxt->seg_reg[seg];
+    struct segment_register *seg_reg;
+
+    if ( seg < 0 || seg >= ARRAY_SIZE(sh_ctxt->seg_reg) )
+        return ERR_PTR(-X86EMUL_UNHANDLEABLE);
+
+    seg_reg = &sh_ctxt->seg_reg[seg];
     if ( !__test_and_set_bit(seg, &sh_ctxt->valid_seg_regs) )
         hvm_get_segment_register(current, seg, seg_reg);
     return seg_reg;
@@ -143,14 +152,9 @@ static int hvm_translate_linear_addr(
     const struct segment_register *reg;
     int okay;
 
-    /*
-     * Can arrive here with non-user segments.  However, no such cirucmstance
-     * is part of a legitimate pagetable update, so fail the emulation.
-     */
-    if ( !is_x86_user_segment(seg) )
-        return X86EMUL_UNHANDLEABLE;
-
     reg = hvm_get_seg_reg(seg, sh_ctxt);
+    if ( IS_ERR(reg) )
+        return -PTR_ERR(reg);
 
     okay = hvm_virtual_to_linear_addr(
         seg, reg, offset, bytes, access_type, sh_ctxt->ctxt.addr_size, paddr);
@@ -253,9 +257,6 @@ hvm_emulate_write(enum x86_segment seg,
     unsigned long addr;
     int rc;
 
-    if ( !is_x86_user_segment(seg) )
-        return X86EMUL_UNHANDLEABLE;
-
     /* How many emulations could we save if we unshadowed on stack writes? */
     if ( seg == x86_seg_ss )
         perfc_incr(shadow_fault_emulate_stack);
@@ -283,7 +284,7 @@ hvm_emulate_cmpxchg(enum x86_segment seg,
     unsigned long addr, old, new;
     int rc;
 
-    if ( !is_x86_user_segment(seg) || bytes > sizeof(long) )
+    if ( bytes > sizeof(long) )
         return X86EMUL_UNHANDLEABLE;
 
     rc = hvm_translate_linear_addr(
diff --git a/xen/arch/x86/mm/shadow/private.h b/xen/arch/x86/mm/shadow/private.h
index 824796f..f0b0ed4 100644
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -740,8 +740,6 @@ const struct x86_emulate_ops *shadow_init_emulation(
     struct sh_emulate_ctxt *sh_ctxt, struct cpu_user_regs *regs);
 void shadow_continue_emulation(
     struct sh_emulate_ctxt *sh_ctxt, struct cpu_user_regs *regs);
-struct segment_register *hvm_get_seg_reg(
-    enum x86_segment seg, struct sh_emulate_ctxt *sh_ctxt);
 
 #if (SHADOW_OPTIMIZATIONS & SHOPT_VIRTUAL_TLB)
 /**************************************************************************/
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index 142d1b6..3aabcbe 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -13,6 +13,7 @@
 #define __ASM_X86_HVM_EMULATE_H__
 
 #include <xen/config.h>
+#include <xen/err.h>
 #include <asm/hvm/hvm.h>
 #include <asm/x86_emulate.h>
 
-- 
2.1.4


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

[PATCH 3/4] x86/hvm: Optimise segment accesses in hvmemul_write_segment()

[Andrew Cooper]

There is no need to read the segment information from VMCS/VMCB and cache it,
just to clobber the cached content immediately afterwards.

Write straight into the cache and set the accessed/dirty bits.

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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 0eb7a4d..e3bfda5 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1447,12 +1447,12 @@ static int hvmemul_write_segment(
 {
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
-    struct segment_register *sreg = hvmemul_get_seg_reg(seg, hvmemul_ctxt);
 
-    if ( IS_ERR(sreg) )
-         return -PTR_ERR(sreg);
+    if ( seg < 0 || seg >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
+        return X86EMUL_UNHANDLEABLE;
 
-    memcpy(sreg, reg, sizeof(struct segment_register));
+    hvmemul_ctxt->seg_reg[seg] = *reg;
+    __set_bit(seg, &hvmemul_ctxt->seg_reg_accessed);
     __set_bit(seg, &hvmemul_ctxt->seg_reg_dirty);
 
     return X86EMUL_OKAY;
-- 
2.1.4


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

Re: [PATCH 3/4] x86/hvm: Optimise segment accesses in hvmemul_write_segment()

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 08 September 2016 15:12
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Paul Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH 3/4] x86/hvm: Optimise segment accesses in
> hvmemul_write_segment()
> 
> There is no need to read the segment information from VMCS/VMCB and
> cache it, just to clobber the cached content immediately afterwards.
> 
> Write straight into the cache and set the accessed/dirty bits.
> 

Yes, the way the code is now does look somewhat silly.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Paul Durrant <paul.durrant@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
>  xen/arch/x86/hvm/emulate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 0eb7a4d..e3bfda5 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1447,12 +1447,12 @@ static int hvmemul_write_segment(  {
>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> -    struct segment_register *sreg = hvmemul_get_seg_reg(seg,
> hvmemul_ctxt);
> 
> -    if ( IS_ERR(sreg) )
> -         return -PTR_ERR(sreg);
> +    if ( seg < 0 || seg >= ARRAY_SIZE(hvmemul_ctxt->seg_reg) )
> +        return X86EMUL_UNHANDLEABLE;
> 
> -    memcpy(sreg, reg, sizeof(struct segment_register));
> +    hvmemul_ctxt->seg_reg[seg] = *reg;
> +    __set_bit(seg, &hvmemul_ctxt->seg_reg_accessed);
>      __set_bit(seg, &hvmemul_ctxt->seg_reg_dirty);
> 
>      return X86EMUL_OKAY;
> --
> 2.1.4


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

Re: [PATCH 3/4] x86/hvm: Optimise segment accesses in hvmemul_write_segment()

[Jan Beulich]

>>> On 08.09.16 at 16:11, <andrew.cooper3@citrix.com> wrote:
> There is no need to read the segment information from VMCS/VMCB and cache it,
> just to clobber the cached content immediately afterwards.
> 
> Write straight into the cache and set the accessed/dirty bits.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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


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

[PATCH 4/4] x86/hvm: Perform a user instruction fetch for a FEP in userspace

[Andrew Cooper]

This matches hardware behaviour, and prevents erroneous failures when a guest
has SMEP/SMAP active and issues a FEP from userspace.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/hvm/hvm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 596a903..159671e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3978,6 +3978,8 @@ void hvm_ud_intercept(struct cpu_user_regs *regs)
     {
         struct vcpu *cur = current;
         const struct segment_register *cs = &ctxt.seg_reg[x86_seg_cs];
+        uint32_t walk = (ctxt.seg_reg[x86_seg_ss].attr.fields.dpl == 3)
+            ? PFEC_user_mode : 0;
         unsigned long addr;
         char sig[5]; /* ud2; .ascii "xen" */
 
@@ -3987,7 +3989,7 @@ void hvm_ud_intercept(struct cpu_user_regs *regs)
                                          cs->attr.fields.l) ? 64 :
                                         cs->attr.fields.db ? 32 : 16, &addr) &&
              (hvm_fetch_from_guest_virt_nofault(sig, addr, sizeof(sig),
-                                                0) == HVMCOPY_okay) &&
+                                                walk) == HVMCOPY_okay) &&
              (memcmp(sig, "\xf\xbxen", sizeof(sig)) == 0) )
         {
             regs->eip += sizeof(sig);
-- 
2.1.4


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

Re: [PATCH 1/4] hvm/fep: Allow testing of instructions crossing the -1 -> 0 virtual boundary

[Jan Beulich]

>>> On 08.09.16 at 16:11, <andrew.cooper3@citrix.com> wrote:
> The Force Emulation Prefix is named to follow its PV counterpart for cpuid or
> rdtsc, but isn't really an instruction prefix.  It behaves as a break-out into
> Xen, with the purpose of emulating the next instruction in the current state.
> 
> It is important to be able to test legal situations which occur in real
> hardware, including instruction which cross certain boundaries, and
> instructions starting at 0.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

While you did mostly convince me at that time, I've got some more
concerns here: What if the instruction to be emulated causes a
fault that then needs to be propagated to and handled by the
guest, before it can be restarted? Such a fault would be raised
with rIP pointing past the forced emulation prefix, and hence the
restarted instruction then wouldn't get emulated.

Along those line, if you don't want to treat this as an instruction
prefix, there ought to be two #DB due to instruction breakpoint
match (if set for both places, of course), yet that's impossible to
implement together with the desire to emulate the insn.

Jan


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

Re: [PATCH 1/4] hvm/fep: Allow testing of instructions crossing the -1 -> 0 virtual boundary

[Andrew Cooper]

On 08/09/16 15:28, Jan Beulich wrote:
>>>> On 08.09.16 at 16:11, <andrew.cooper3@citrix.com> wrote:
>> The Force Emulation Prefix is named to follow its PV counterpart for cpuid or
>> rdtsc, but isn't really an instruction prefix.  It behaves as a break-out into
>> Xen, with the purpose of emulating the next instruction in the current state.
>>
>> It is important to be able to test legal situations which occur in real
>> hardware, including instruction which cross certain boundaries, and
>> instructions starting at 0.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> While you did mostly convince me at that time, I've got some more
> concerns here: What if the instruction to be emulated causes a
> fault that then needs to be propagated to and handled by the
> guest, before it can be restarted? Such a fault would be raised
> with rIP pointing past the forced emulation prefix, and hence the
> restarted instruction then wouldn't get emulated.

The current behaviour is to report a fault at the start of the real
instruction.

Furthermore, this is the useful behaviour for it to have.  If a guest is
explicitly probing the Xen x86 emulator with FEP, it can take
responsibility of rewinding %rip by 5 if it needs to replay.

Having said that, I haven't yet encountered a case where replaying a
faulting instruction in a test is useful.  All tests thusfar check that
in specific situations, faults occur architecturally whether run on real
hardware, or via the xen emulator.

> Along those line, if you don't want to treat this as an instruction
> prefix, there ought to be two #DB due to instruction breakpoint
> match (if set for both places, of course), yet that's impossible to
> implement together with the desire to emulate the insn.

True, but I don't see this is a problem.

The *only* code using FEP is test code deliberately trying to elicit
behaviour from the Xen emulator and check that it matches real
hardware.  It is perfectly fine for test code to know its special when
it is using a special backdoor to perform said tests.

~Andrew

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