[PATCH 00/10] x86 emulation bugfixes and fuzzer improvements

[PATCH 00/10] x86 emulation bugfixes and fuzzer improvements

[Andrew Cooper]

Andrew Cooper (10):
  x86/emul: Correct the decoding of vlddqu
  x86/emul: Add feature check for clzero
  tools/insn-fuzz: Don't use memcpy() for zero-length reads
  tools/insn-fuzz: Avoid making use of static data
  tools/insn-fuzz: Fix a stability bug in afl-clang-fast mode
  tools/insn-fuzz: Correct hook prototypes, and assert() appropriate
    segments
  tools/insn-fuzz: Provide IA32_DEBUGCTL consistently to the emulator
  tools/insn-fuzz: Fix assertion failures in x86_emulate_wrapper()
  tools/x86emul: Advertise more CPUID features for testing purposes
  tools/insn-fuzz: Always use x86_swint_emulate_all

 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 272 +++++++++++++++---------
 tools/tests/x86_emulator/x86_emulate.c          |  41 ++--
 xen/arch/x86/x86_emulate/x86_emulate.c          |  26 ++-
 3 files changed, 228 insertions(+), 111 deletions(-)

-- 
2.1.4


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

[PATCH 01/10] x86/emul: Correct the decoding of vlddqu

[Andrew Cooper]

vlddqu is encoded with 0xf2 which causes it to fall into the Scalar general
case in x86_decode_twobyte().  However, it really does have just two operands,
so must remain TwoOp

AFL discovered that the instruction c5 5b f0 3c e5 95 0a cd 63 was considered
valid despite it being a two operand instruction and VEX.vvvv having the value
11.  The resulting use in a stub yielded #UD.

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

From manually decoding the instruciton, I believe Xen's interpretation of
disp32(none*8) is correct.  binutils 2.25 (Debian Jessie) yields:

   0:       c5 5b f0 3c e5 95 0a        vlddqu 0x63cd0a95(,%riz,8),%xmm15
   7:       cd 63

where it has accounted for disp32 in its decode of instruction, but failed to
properly move its instruction pointer on.

Intel XED OTOH simply gives up with:

  ERROR: GENERAL_ERROR Could not decode at offset: 0x0 PC: 0x0:
  [C55BF03CE5950ACD63000000000000]
---
 xen/arch/x86/x86_emulate/x86_emulate.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index bb67be6..497cc77 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2310,7 +2310,8 @@ x86_decode_twobyte(
     case 0x7f:
     case 0xc2 ... 0xc3:
     case 0xc5 ... 0xc6:
-    case 0xd0 ... 0xfe:
+    case 0xd0 ... 0xef:
+    case 0xf1 ... 0xfe:
         ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);
         break;
 
@@ -2332,9 +2333,9 @@ x86_decode_twobyte(
         if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */
         {
     case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */
-            state->desc = DstImplicit | SrcMem | Mov;
+            state->desc = DstImplicit | SrcMem | TwoOp;
             state->simd_size = simd_other;
-            /* Avoid the state->desc adjustment below. */
+            /* Avoid the state->desc clobbering of TwoOp below. */
             return X86EMUL_OKAY;
         }
         break;
@@ -2374,11 +2375,25 @@ x86_decode_twobyte(
     case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */
         state->desc = DstReg | SrcMem16;
         break;
+
+    case 0xf0:
+        ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);
+        if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */
+        {
+        /* fall through */
+    case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */
+            state->desc = DstImplicit | SrcMem | TwoOp;
+            state->simd_size = simd_other;
+            /* Avoid the state->desc clobbering of TwoOp below. */
+            return X86EMUL_OKAY;
+        }
+        break;
     }
 
     /*
      * Scalar forms of most VEX-encoded TwoOp instructions have
-     * three operands.
+     * three operands.  Those which do really have two operands
+     * should have exited earlier.
      */
     if ( state->simd_size && vex.opcx &&
          (vex.pfx & VEX_PREFIX_SCALAR_MASK) )
-- 
2.1.4


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

[PATCH 02/10] x86/emul: Add feature check for clzero

[Andrew Cooper]

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

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 497cc77..7af8a42 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1604,6 +1604,7 @@ static bool vcpu_has(
 #define vcpu_has_clwb()        vcpu_has(         7, EBX, 24, ctxt, ops)
 #define vcpu_has_sha()         vcpu_has(         7, EBX, 29, ctxt, ops)
 #define vcpu_has_rdpid()       vcpu_has(         7, ECX, 22, ctxt, ops)
+#define vcpu_has_clzero()      vcpu_has(0x80000008, EBX,  0, ctxt, ops)
 
 #define vcpu_must_have(feat) \
     generate_exception_if(!vcpu_has_##feat(), EXC_UD)
@@ -5183,6 +5184,8 @@ x86_emulate(
         {
             unsigned long zero = 0;
 
+            vcpu_must_have(clzero);
+
             base = ad_bytes == 8 ? _regs.r(ax) :
                    ad_bytes == 4 ? _regs.eax : _regs.ax;
             limit = 0;
-- 
2.1.4


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

[PATCH 03/10] tools/insn-fuzz: Don't use memcpy() for zero-length reads

[Andrew Cooper]

For control-flow changes, the emulator needs to perform a zero-length
instruction fetch at the target offset.  It also passes NULL for the
destination buffer, as there is no instruction stream to collect.

This trips up UBSAN, even with a size of 0.  Exclude zero-length reads from
using memcpy(), rather than excluding NULL destination pointers, to still
catch unintentional uses of NULL.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 890642c..cbdb3dd 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -77,7 +77,7 @@ static int data_read(const char *why, void *dst, unsigned int bytes)
     else
         rc = maybe_fail(why, true);
 
-    if ( rc == X86EMUL_OKAY )
+    if ( rc == X86EMUL_OKAY && bytes )
     {
         memcpy(dst,  input.data + data_index, bytes);
         data_index += bytes;
-- 
2.1.4


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

[PATCH 04/10] tools/insn-fuzz: Avoid making use of static data

[Andrew Cooper]

AFL has a measure of stability, where it passes the same corpus into the
fuzzing harness and observes whether the execution path changes from before.
Any instability in the fuzzing harness reduces its effectiveness, as an
observed crash may not reliably be caused by the original corpus.

In preparation to fix a stability bug, introduce struct fuzz_state, allocated
on the stack and passed around via struct x86_emulate_ctxt's data parameter.
Propagate ctxt into the helpers such as maybe_fail(), so the state can be
retrieved.

Move the previously-static data_{index,num} into struct fuzz_state.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 183 +++++++++++++++---------
 1 file changed, 118 insertions(+), 65 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index cbdb3dd..907275b 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -21,7 +21,9 @@
 
 #define SEG_NUM x86_seg_none
 
-struct input_struct {
+/* Layout of data expected as fuzzing input. */
+struct fuzz_corpus
+{
     unsigned long cr[5];
     uint64_t msr[MSR_INDEX_MAX];
     struct cpu_user_regs regs;
@@ -29,19 +31,36 @@ struct input_struct {
     unsigned long options;
     unsigned char data[4096];
 } input;
-#define DATA_OFFSET offsetof(struct input_struct, data)
-static unsigned int data_index;
-static unsigned int data_num;
+#define DATA_OFFSET offsetof(struct fuzz_corpus, data)
+
+/*
+ * Internal state of the fuzzing harness.  Calculated initially from the input
+ * corpus, and later mutates by the emulation callbacks.
+ */
+struct fuzz_state
+{
+    /* Fuzzer's input data. */
+    struct fuzz_corpus *corpus;
+
+    /* Real amount of data backing corpus->data[]. */
+    size_t data_num;
+
+    /* Amount of corpus->data[] consumed thus far. */
+    size_t data_index;
+};
 
 /*
  * Randomly return success or failure when processing data.  If
  * `exception` is false, this function turns _EXCEPTION to _OKAY.
  */
-static int maybe_fail(const char *why, bool exception)
+static int maybe_fail(struct x86_emulate_ctxt *ctxt,
+                      const char *why, bool exception)
 {
+    struct fuzz_state *s = ctxt->data;
+    const struct fuzz_corpus *c = s->corpus;
     int rc;
 
-    if ( data_index >= data_num )
+    if ( s->data_index >= s->data_num )
         rc = X86EMUL_EXCEPTION;
     else
     {
@@ -50,13 +69,13 @@ static int maybe_fail(const char *why, bool exception)
          * 25% unhandlable
          * 25% exception
          */
-        if ( input.data[data_index] > 0xc0 )
+        if ( c->data[s->data_index] > 0xc0 )
             rc = X86EMUL_EXCEPTION;
-        else if ( input.data[data_index] > 0x80 )
+        else if ( c->data[s->data_index] > 0x80 )
             rc = X86EMUL_UNHANDLEABLE;
         else
             rc = X86EMUL_OKAY;
-        data_index++;
+        s->data_index++;
     }
 
     if ( rc == X86EMUL_EXCEPTION && !exception )
@@ -67,20 +86,23 @@ static int maybe_fail(const char *why, bool exception)
     return rc;
 }
 
-static int data_read(const char *why, void *dst, unsigned int bytes)
+static int data_read(struct x86_emulate_ctxt *ctxt,
+                     const char *why, void *dst, unsigned int bytes)
 {
+    struct fuzz_state *s = ctxt->data;
+    const struct fuzz_corpus *c = s->corpus;
     unsigned int i;
     int rc;
 
-    if ( data_index + bytes > data_num )
+    if ( s->data_index + bytes > s->data_num )
         rc = X86EMUL_EXCEPTION;
     else
-        rc = maybe_fail(why, true);
+        rc = maybe_fail(ctxt, why, true);
 
     if ( rc == X86EMUL_OKAY && bytes )
     {
-        memcpy(dst,  input.data + data_index, bytes);
-        data_index += bytes;
+        memcpy(dst, &c->data[s->data_index], bytes);
+        s->data_index += bytes;
 
         printf("%s: ", why);
         for ( i = 0; i < bytes; i++ )
@@ -98,7 +120,7 @@ static int fuzz_read(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
-    return data_read("read", p_data, bytes);
+    return data_read(ctxt, "read", p_data, bytes);
 }
 
 static int fuzz_read_io(
@@ -107,7 +129,7 @@ static int fuzz_read_io(
     unsigned long *val,
     struct x86_emulate_ctxt *ctxt)
 {
-    return data_read("read_io", val, bytes);
+    return data_read(ctxt, "read_io", val, bytes);
 }
 
 static int fuzz_insn_fetch(
@@ -117,15 +139,16 @@ static int fuzz_insn_fetch(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
-    return data_read("insn_fetch", p_data, bytes);
+    return data_read(ctxt, "insn_fetch", p_data, bytes);
 }
 
-static int _fuzz_rep_read(const char *why, unsigned long *reps)
+static int _fuzz_rep_read(struct x86_emulate_ctxt *ctxt,
+                          const char *why, unsigned long *reps)
 {
     int rc;
     unsigned long bytes_read = 0;
 
-    rc = data_read(why, &bytes_read, sizeof(bytes_read));
+    rc = data_read(ctxt, why, &bytes_read, sizeof(bytes_read));
 
     if ( bytes_read <= *reps )
         *reps = bytes_read;
@@ -146,9 +169,10 @@ static int _fuzz_rep_read(const char *why, unsigned long *reps)
     return rc;
 }
 
-static int _fuzz_rep_write(const char *why, unsigned long *reps)
+static int _fuzz_rep_write(struct x86_emulate_ctxt *ctxt,
+                           const char *why, unsigned long *reps)
 {
-    int rc = maybe_fail(why, true);
+    int rc = maybe_fail(ctxt, why, true);
 
     switch ( rc )
     {
@@ -174,7 +198,7 @@ static int fuzz_rep_ins(
     unsigned long *reps,
     struct x86_emulate_ctxt *ctxt)
 {
-    return _fuzz_rep_read("rep_ins", reps);
+    return _fuzz_rep_read(ctxt, "rep_ins", reps);
 }
 
 static int fuzz_rep_movs(
@@ -186,7 +210,7 @@ static int fuzz_rep_movs(
     unsigned long *reps,
     struct x86_emulate_ctxt *ctxt)
 {
-    return _fuzz_rep_read("rep_movs", reps);
+    return _fuzz_rep_read(ctxt, "rep_movs", reps);
 }
 
 static int fuzz_rep_outs(
@@ -197,7 +221,7 @@ static int fuzz_rep_outs(
     unsigned long *reps,
     struct x86_emulate_ctxt *ctxt)
 {
-    return _fuzz_rep_write("rep_outs", reps);
+    return _fuzz_rep_write(ctxt, "rep_outs", reps);
 }
 
 static int fuzz_rep_stos(
@@ -208,7 +232,7 @@ static int fuzz_rep_stos(
     unsigned long *reps,
     struct x86_emulate_ctxt *ctxt)
 {
-    return _fuzz_rep_write("rep_stos", reps);
+    return _fuzz_rep_write(ctxt, "rep_stos", reps);
 }
 
 static int fuzz_write(
@@ -218,7 +242,7 @@ static int fuzz_write(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
-    return maybe_fail("write", true);
+    return maybe_fail(ctxt, "write", true);
 }
 
 static int fuzz_cmpxchg(
@@ -229,7 +253,7 @@ static int fuzz_cmpxchg(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
-    return maybe_fail("cmpxchg", true);
+    return maybe_fail(ctxt, "cmpxchg", true);
 }
 
 static int fuzz_invlpg(
@@ -237,13 +261,13 @@ static int fuzz_invlpg(
     unsigned long offset,
     struct x86_emulate_ctxt *ctxt)
 {
-    return maybe_fail("invlpg", false);
+    return maybe_fail(ctxt, "invlpg", false);
 }
 
 static int fuzz_wbinvd(
     struct x86_emulate_ctxt *ctxt)
 {
-    return maybe_fail("wbinvd", true);
+    return maybe_fail(ctxt, "wbinvd", true);
 }
 
 static int fuzz_write_io(
@@ -252,7 +276,7 @@ static int fuzz_write_io(
     unsigned long val,
     struct x86_emulate_ctxt *ctxt)
 {
-    return maybe_fail("write_io", true);
+    return maybe_fail(ctxt, "write_io", true);
 }
 
 static int fuzz_read_segment(
@@ -260,10 +284,13 @@ static int fuzz_read_segment(
     struct segment_register *reg,
     struct x86_emulate_ctxt *ctxt)
 {
+    struct fuzz_state *s = ctxt->data;
+    const struct fuzz_corpus *c = s->corpus;
+
     if ( seg >= SEG_NUM )
         return X86EMUL_UNHANDLEABLE;
 
-    *reg = input.segments[seg];
+    *reg = c->segments[seg];
 
     return X86EMUL_OKAY;
 }
@@ -273,15 +300,17 @@ static int fuzz_write_segment(
     const struct segment_register *reg,
     struct x86_emulate_ctxt *ctxt)
 {
+    struct fuzz_state *s = ctxt->data;
+    struct fuzz_corpus *c = s->corpus;
     int rc;
 
     if ( seg >= SEG_NUM )
         return X86EMUL_UNHANDLEABLE;
 
-    rc = maybe_fail("write_segment", true);
+    rc = maybe_fail(ctxt, "write_segment", true);
 
     if ( rc == X86EMUL_OKAY )
-        input.segments[seg] = *reg;
+        c->segments[seg] = *reg;
 
     return rc;
 }
@@ -291,10 +320,13 @@ static int fuzz_read_cr(
     unsigned long *val,
     struct x86_emulate_ctxt *ctxt)
 {
-    if ( reg >= ARRAY_SIZE(input.cr) )
+    struct fuzz_state *s = ctxt->data;
+    const struct fuzz_corpus *c = s->corpus;
+
+    if ( reg >= ARRAY_SIZE(c->cr) )
         return X86EMUL_UNHANDLEABLE;
 
-    *val = input.cr[reg];
+    *val = c->cr[reg];
 
     return X86EMUL_OKAY;
 }
@@ -304,16 +336,18 @@ static int fuzz_write_cr(
     unsigned long val,
     struct x86_emulate_ctxt *ctxt)
 {
+    struct fuzz_state *s = ctxt->data;
+    struct fuzz_corpus *c = s->corpus;
     int rc;
 
-    if ( reg >= ARRAY_SIZE(input.cr) )
+    if ( reg >= ARRAY_SIZE(c->cr) )
         return X86EMUL_UNHANDLEABLE;
 
-    rc = maybe_fail("write_cr", true);
+    rc = maybe_fail(ctxt, "write_cr", true);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    input.cr[reg] = val;
+    c->cr[reg] = val;
 
     return X86EMUL_OKAY;
 }
@@ -345,6 +379,8 @@ static int fuzz_read_msr(
     uint64_t *val,
     struct x86_emulate_ctxt *ctxt)
 {
+    struct fuzz_state *s = ctxt->data;
+    const struct fuzz_corpus *c = s->corpus;
     unsigned int idx;
 
     switch ( reg )
@@ -356,12 +392,12 @@ static int fuzz_read_msr(
          * should preferably return consistent values, but returning
          * random values is fine in fuzzer.
          */
-        return data_read("read_msr", val, sizeof(*val));
+        return data_read(ctxt, "read_msr", val, sizeof(*val));
     case MSR_EFER:
-        *val = input.msr[MSRI_EFER];
+        *val = c->msr[MSRI_EFER];
         *val &= ~EFER_LMA;
-        if ( (*val & EFER_LME) && (input.cr[4] & X86_CR4_PAE) &&
-             (input.cr[0] & X86_CR0_PG) )
+        if ( (*val & EFER_LME) && (c->cr[4] & X86_CR4_PAE) &&
+             (c->cr[0] & X86_CR0_PG) )
         {
             printf("Setting EFER_LMA\n");
             *val |= EFER_LMA;
@@ -373,7 +409,7 @@ static int fuzz_read_msr(
     {
         if ( msr_index[idx] == reg )
         {
-            *val = input.msr[idx];
+            *val = c->msr[idx];
             return X86EMUL_OKAY;
         }
     }
@@ -386,10 +422,12 @@ static int fuzz_write_msr(
     uint64_t val,
     struct x86_emulate_ctxt *ctxt)
 {
+    struct fuzz_state *s = ctxt->data;
+    struct fuzz_corpus *c = s->corpus;
     unsigned int idx;
     int rc;
 
-    rc = maybe_fail("write_msr", true);
+    rc = maybe_fail(ctxt, "write_msr", true);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
@@ -404,7 +442,7 @@ static int fuzz_write_msr(
     {
         if ( msr_index[idx] == reg )
         {
-            input.msr[idx] = val;
+            c->msr[idx] = val;
             return X86EMUL_OKAY;
         }
     }
@@ -452,14 +490,16 @@ static void setup_fpu_exception_handler(void)
 
 static void dump_state(struct x86_emulate_ctxt *ctxt)
 {
+    struct fuzz_state *s = ctxt->data;
+    const struct fuzz_corpus *c = s->corpus;
     struct cpu_user_regs *regs = ctxt->regs;
     uint64_t val = 0;
 
     printf(" -- State -- \n");
     printf("addr / sp size: %d / %d\n", ctxt->addr_size, ctxt->sp_size);
-    printf(" cr0: %lx\n", input.cr[0]);
-    printf(" cr3: %lx\n", input.cr[3]);
-    printf(" cr4: %lx\n", input.cr[4]);
+    printf(" cr0: %lx\n", c->cr[0]);
+    printf(" cr3: %lx\n", c->cr[3]);
+    printf(" cr4: %lx\n", c->cr[4]);
 
     printf(" rip: %"PRIx64"\n", regs->rip);
 
@@ -479,17 +519,23 @@ static bool long_mode_active(struct x86_emulate_ctxt *ctxt)
 
 static bool in_longmode(struct x86_emulate_ctxt *ctxt)
 {
-    return long_mode_active(ctxt) && input.segments[x86_seg_cs].attr.fields.l;
+    struct fuzz_state *s = ctxt->data;
+    const struct fuzz_corpus *c = s->corpus;
+
+    return long_mode_active(ctxt) && c->segments[x86_seg_cs].attr.fields.l;
 }
 
 static void set_sizes(struct x86_emulate_ctxt *ctxt)
 {
+    struct fuzz_state *s = ctxt->data;
+    const struct fuzz_corpus *c = s->corpus;
+
     if ( in_longmode(ctxt) )
         ctxt->addr_size = ctxt->sp_size = 64;
     else
     {
-        ctxt->addr_size = input.segments[x86_seg_cs].attr.fields.db ? 32 : 16;
-        ctxt->sp_size   = input.segments[x86_seg_ss].attr.fields.db ? 32 : 16;
+        ctxt->addr_size = c->segments[x86_seg_cs].attr.fields.db ? 32 : 16;
+        ctxt->sp_size   = c->segments[x86_seg_ss].attr.fields.db ? 32 : 16;
     }
 }
 
@@ -550,9 +596,11 @@ enum {
         printf("Disabling hook "#h"\n");               \
     }
 
-static void disable_hooks(void)
+static void disable_hooks(struct x86_emulate_ctxt *ctxt)
 {
-    unsigned long bitmap = input.options;
+    struct fuzz_state *s = ctxt->data;
+    const struct fuzz_corpus *c = s->corpus;
+    unsigned long bitmap = c->options;
 
     /* See also sanitize_input, some hooks can't be disabled. */
     MAYBE_DISABLE_HOOK(read);
@@ -579,7 +627,9 @@ static void disable_hooks(void)
 
 static void set_swint_support(struct x86_emulate_ctxt *ctxt)
 {
-    unsigned int swint_opt = (input.options >> OPTION_swint_emulation) & 3;
+    struct fuzz_state *s = ctxt->data;
+    struct fuzz_corpus *c = s->corpus;
+    unsigned int swint_opt = (c->options >> OPTION_swint_emulation) & 3;
     static const enum x86_swint_emulation map[4] = {
         x86_swint_emulate_none,
         x86_swint_emulate_none,
@@ -614,11 +664,13 @@ static void set_swint_support(struct x86_emulate_ctxt *ctxt)
  */
 static void sanitize_input(struct x86_emulate_ctxt *ctxt)
 {
-    struct cpu_user_regs *regs = &input.regs;
-    unsigned long bitmap = input.options;
+    struct fuzz_state *s = ctxt->data;
+    struct fuzz_corpus *c = s->corpus;
+    struct cpu_user_regs *regs = &c->regs;
+    unsigned long bitmap = c->options;
 
     /* Some hooks can't be disabled. */
-    input.options &= ~((1<<HOOK_read)|(1<<HOOK_insn_fetch));
+    c->options &= ~((1<<HOOK_read)|(1<<HOOK_insn_fetch));
 
     /* Zero 'private' entries */
     regs->error_code = 0;
@@ -632,8 +684,8 @@ static void sanitize_input(struct x86_emulate_ctxt *ctxt)
      * CR0.PG can't be set if CR0.PE isn't set.  Set is more interesting, so
      * set PE if PG is set.
      */
-    if ( input.cr[0] & X86_CR0_PG )
-        input.cr[0] |= X86_CR0_PE;
+    if ( c->cr[0] & X86_CR0_PG )
+        c->cr[0] |= X86_CR0_PE;
 
     /* EFLAGS.VM not available in long mode */
     if ( long_mode_active(ctxt) )
@@ -642,8 +694,8 @@ static void sanitize_input(struct x86_emulate_ctxt *ctxt)
     /* EFLAGS.VM implies 16-bit mode */
     if ( regs->rflags & X86_EFLAGS_VM )
     {
-        input.segments[x86_seg_cs].attr.fields.db = 0;
-        input.segments[x86_seg_ss].attr.fields.db = 0;
+        c->segments[x86_seg_cs].attr.fields.db = 0;
+        c->segments[x86_seg_ss].attr.fields.db = 0;
     }
 }
 
@@ -661,7 +713,9 @@ int LLVMFuzzerInitialize(int *argc, char ***argv)
 int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
 {
     struct cpu_user_regs regs = {};
+    struct fuzz_state state = {};
     struct x86_emulate_ctxt ctxt = {
+        .data = &state,
         .regs = &regs,
         .addr_size = 8 * sizeof(void *),
         .sp_size = 8 * sizeof(void *),
@@ -670,8 +724,6 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
 
     /* Reset all global state variables */
     memset(&input, 0, sizeof(input));
-    data_index = 0;
-    data_num = 0;
 
     if ( size <= DATA_OFFSET )
     {
@@ -687,11 +739,12 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
 
     memcpy(&input, data_p, size);
 
-    data_num = size - DATA_OFFSET;
+    state.corpus = &input;
+    state.data_num = size - DATA_OFFSET;
 
     sanitize_input(&ctxt);
 
-    disable_hooks();
+    disable_hooks(&ctxt);
 
     set_swint_support(&ctxt);
 
-- 
2.1.4


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

[PATCH 05/10] tools/insn-fuzz: Fix a stability bug in afl-clang-fast mode

[Andrew Cooper]

The fuzzing harness conditionally disables hooks to test error paths in the
emulator.  However, fuzz_emulops is a static structure.

c/s 69f4633 "tools/insn-fuzz: Support AFL's afl-clang-fast mode" introduced
persistent mode, but because fuzz_emulops is static, the clobbering of hooks
accumulates over repeated input, meaning that previous corpora influence the
execution over the current corpus.

Move the partially clobbered struct x86_emulate_ops into struct fuzz_state,
which is re-initialised from full on each call to LLVMFuzzerTestOneInput()

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 907275b..06d7cdc 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -47,6 +47,9 @@ struct fuzz_state
 
     /* Amount of corpus->data[] consumed thus far. */
     size_t data_index;
+
+    /* Emulation ops, some of which are disabled based on corpus->options. */
+    struct x86_emulate_ops ops;
 };
 
 /*
@@ -451,7 +454,7 @@ static int fuzz_write_msr(
 }
 
 #define SET(h) .h = fuzz_##h
-static struct x86_emulate_ops fuzz_emulops = {
+static const struct x86_emulate_ops all_fuzzer_ops = {
     SET(read),
     SET(insn_fetch),
     SET(write),
@@ -592,7 +595,7 @@ enum {
 #define MAYBE_DISABLE_HOOK(h)                          \
     if ( bitmap & (1 << HOOK_##h) )                    \
     {                                                  \
-        fuzz_emulops.h = NULL;                         \
+        s->ops.h = NULL;                               \
         printf("Disabling hook "#h"\n");               \
     }
 
@@ -713,7 +716,9 @@ int LLVMFuzzerInitialize(int *argc, char ***argv)
 int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
 {
     struct cpu_user_regs regs = {};
-    struct fuzz_state state = {};
+    struct fuzz_state state = {
+        .ops = all_fuzzer_ops,
+    };
     struct x86_emulate_ctxt ctxt = {
         .data = &state,
         .regs = &regs,
@@ -755,7 +760,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
         set_sizes(&ctxt);
         dump_state(&ctxt);
 
-        rc = x86_emulate(&ctxt, &fuzz_emulops);
+        rc = x86_emulate(&ctxt, &state.ops);
         printf("Emulation result: %d\n", rc);
     } while ( rc == X86EMUL_OKAY );
 
-- 
2.1.4


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

[PATCH 06/10] tools/insn-fuzz: Correct hook prototypes, and assert() appropriate segments

[Andrew Cooper]

The correct prototypes for the hooks are to use enum x86_segment rather than
unsigned int.  It is implementation specific as to whether this compiles.

assert() that the emulator never passes an inappropriate segment.  The only
hook which may legitimately be passed x86_seg_none is invlpg().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 38 +++++++++++++++++++------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 06d7cdc..74c15d2 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -117,12 +117,14 @@ static int data_read(struct x86_emulate_ctxt *ctxt,
 }
 
 static int fuzz_read(
-    unsigned int seg,
+    enum x86_segment seg,
     unsigned long offset,
     void *p_data,
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    assert((unsigned int)seg < x86_seg_none);
+
     return data_read(ctxt, "read", p_data, bytes);
 }
 
@@ -136,12 +138,14 @@ static int fuzz_read_io(
 }
 
 static int fuzz_insn_fetch(
-    unsigned int seg,
+    enum x86_segment seg,
     unsigned long offset,
     void *p_data,
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    assert(seg == x86_seg_cs);
+
     return data_read(ctxt, "insn_fetch", p_data, bytes);
 }
 
@@ -201,6 +205,8 @@ static int fuzz_rep_ins(
     unsigned long *reps,
     struct x86_emulate_ctxt *ctxt)
 {
+    assert(dst_seg == x86_seg_es);
+
     return _fuzz_rep_read(ctxt, "rep_ins", reps);
 }
 
@@ -213,6 +219,9 @@ static int fuzz_rep_movs(
     unsigned long *reps,
     struct x86_emulate_ctxt *ctxt)
 {
+    assert(is_x86_user_segment(src_seg));
+    assert(dst_seg == x86_seg_es);
+
     return _fuzz_rep_read(ctxt, "rep_movs", reps);
 }
 
@@ -224,6 +233,8 @@ static int fuzz_rep_outs(
     unsigned long *reps,
     struct x86_emulate_ctxt *ctxt)
 {
+    assert(is_x86_user_segment(src_seg));
+
     return _fuzz_rep_write(ctxt, "rep_outs", reps);
 }
 
@@ -235,27 +246,37 @@ static int fuzz_rep_stos(
     unsigned long *reps,
     struct x86_emulate_ctxt *ctxt)
 {
+    /*
+     * STOS itself may only have an %es segment, but the stos() hook is reused
+     * for CLZERO.
+     */
+    assert(is_x86_user_segment(seg));
+
     return _fuzz_rep_write(ctxt, "rep_stos", reps);
 }
 
 static int fuzz_write(
-    unsigned int seg,
+    enum x86_segment seg,
     unsigned long offset,
     void *p_data,
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    assert((unsigned int)seg < x86_seg_none);
+
     return maybe_fail(ctxt, "write", true);
 }
 
 static int fuzz_cmpxchg(
-    unsigned int seg,
+    enum x86_segment seg,
     unsigned long offset,
     void *old,
     void *new,
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    assert((unsigned int)seg < x86_seg_none);
+
     return maybe_fail(ctxt, "cmpxchg", true);
 }
 
@@ -264,6 +285,9 @@ static int fuzz_invlpg(
     unsigned long offset,
     struct x86_emulate_ctxt *ctxt)
 {
+    /* invlpg(), unlike all other hooks, may be called with x86_seg_none. */
+    assert((unsigned int)seg <= x86_seg_none);
+
     return maybe_fail(ctxt, "invlpg", false);
 }
 
@@ -290,8 +314,7 @@ static int fuzz_read_segment(
     struct fuzz_state *s = ctxt->data;
     const struct fuzz_corpus *c = s->corpus;
 
-    if ( seg >= SEG_NUM )
-        return X86EMUL_UNHANDLEABLE;
+    assert((unsigned int)seg < x86_seg_none);
 
     *reg = c->segments[seg];
 
@@ -307,8 +330,7 @@ static int fuzz_write_segment(
     struct fuzz_corpus *c = s->corpus;
     int rc;
 
-    if ( seg >= SEG_NUM )
-        return X86EMUL_UNHANDLEABLE;
+    assert((unsigned int)seg < x86_seg_none);
 
     rc = maybe_fail(ctxt, "write_segment", true);
 
-- 
2.1.4


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

[PATCH 07/10] tools/insn-fuzz: Provide IA32_DEBUGCTL consistently to the emulator

[Andrew Cooper]

x86_emulates()'s is_branch_step() performs a speculative read of
IA32_DEBUGCTL, but doesn't squash exceptions should they arise.  In reality,
this MSR is always available.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>

RFC: I'm also wondering whether it would be better for the emulator to always
clean up after failed speculative reads.  It is plausible that might wish to
explicitly run with some architectural MSRs unavailable.
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 74c15d2..ca902f6 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -385,7 +385,8 @@ enum {
     MSRI_STAR,
     MSRI_LSTAR,
     MSRI_CSTAR,
-    MSRI_SYSCALL_MASK
+    MSRI_SYSCALL_MASK,
+    MSRI_IA32_DEBUGCTLMSR,
 };
 
 static const unsigned int msr_index[MSR_INDEX_MAX] = {
@@ -396,7 +397,8 @@ static const unsigned int msr_index[MSR_INDEX_MAX] = {
     [MSRI_STAR]              = MSR_STAR,
     [MSRI_LSTAR]             = MSR_LSTAR,
     [MSRI_CSTAR]             = MSR_CSTAR,
-    [MSRI_SYSCALL_MASK]      = MSR_SYSCALL_MASK
+    [MSRI_SYSCALL_MASK]      = MSR_SYSCALL_MASK,
+    [MSRI_IA32_DEBUGCTLMSR]  = MSR_IA32_DEBUGCTLMSR,
 };
 
 static int fuzz_read_msr(
-- 
2.1.4


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

[PATCH 08/10] tools/insn-fuzz: Fix assertion failures in x86_emulate_wrapper()

[Andrew Cooper]

c/s 92cf67888 "x86/emul: Hold x86_emulate() to strict X86EMUL_EXCEPTION
requirements" was appropriate for the hypervisor, but the fuzzer stubs didn't
conform to the stricter requirements.  AFL is very quick to discover this.

Extend the fuzzing harness exception logic to raise exceptions appropriately.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 27 ++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index ca902f6..1906186 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -86,10 +86,15 @@ static int maybe_fail(struct x86_emulate_ctxt *ctxt,
 
     printf("maybe_fail %s: %d\n", why, rc);
 
+    if ( rc == X86EMUL_EXCEPTION )
+        /* Fake up a pagefault. */
+        x86_emul_pagefault(0, 0, ctxt);
+
     return rc;
 }
 
 static int data_read(struct x86_emulate_ctxt *ctxt,
+                     enum x86_segment seg,
                      const char *why, void *dst, unsigned int bytes)
 {
     struct fuzz_state *s = ctxt->data;
@@ -98,7 +103,17 @@ static int data_read(struct x86_emulate_ctxt *ctxt,
     int rc;
 
     if ( s->data_index + bytes > s->data_num )
+    {
+        /*
+         * Fake up a segment limit violation.  System segment limit volations
+         * are reported by X86EMUL_EXCEPTION alone, so the emulator can fill
+         * in the correct context.
+         */
+        if ( !is_x86_system_segment(seg) )
+            x86_emul_hw_exception(13, 0, ctxt);
+
         rc = X86EMUL_EXCEPTION;
+    }
     else
         rc = maybe_fail(ctxt, why, true);
 
@@ -125,7 +140,7 @@ static int fuzz_read(
 {
     assert((unsigned int)seg < x86_seg_none);
 
-    return data_read(ctxt, "read", p_data, bytes);
+    return data_read(ctxt, seg, "read", p_data, bytes);
 }
 
 static int fuzz_read_io(
@@ -134,7 +149,7 @@ static int fuzz_read_io(
     unsigned long *val,
     struct x86_emulate_ctxt *ctxt)
 {
-    return data_read(ctxt, "read_io", val, bytes);
+    return data_read(ctxt, x86_seg_none, "read_io", val, bytes);
 }
 
 static int fuzz_insn_fetch(
@@ -146,7 +161,7 @@ static int fuzz_insn_fetch(
 {
     assert(seg == x86_seg_cs);
 
-    return data_read(ctxt, "insn_fetch", p_data, bytes);
+    return data_read(ctxt, seg, "insn_fetch", p_data, bytes);
 }
 
 static int _fuzz_rep_read(struct x86_emulate_ctxt *ctxt,
@@ -155,7 +170,7 @@ static int _fuzz_rep_read(struct x86_emulate_ctxt *ctxt,
     int rc;
     unsigned long bytes_read = 0;
 
-    rc = data_read(ctxt, why, &bytes_read, sizeof(bytes_read));
+    rc = data_read(ctxt, x86_seg_none, why, &bytes_read, sizeof(bytes_read));
 
     if ( bytes_read <= *reps )
         *reps = bytes_read;
@@ -419,7 +434,7 @@ static int fuzz_read_msr(
          * should preferably return consistent values, but returning
          * random values is fine in fuzzer.
          */
-        return data_read(ctxt, "read_msr", val, sizeof(*val));
+        return data_read(ctxt, x86_seg_none, "read_msr", val, sizeof(*val));
     case MSR_EFER:
         *val = c->msr[MSRI_EFER];
         *val &= ~EFER_LMA;
@@ -441,6 +456,7 @@ static int fuzz_read_msr(
         }
     }
 
+    x86_emul_hw_exception(13, 0, ctxt);
     return X86EMUL_EXCEPTION;
 }
 
@@ -474,6 +490,7 @@ static int fuzz_write_msr(
         }
     }
 
+    x86_emul_hw_exception(13, 0, ctxt);
     return X86EMUL_EXCEPTION;
 }
 
-- 
2.1.4


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

[PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[Andrew Cooper]

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c
index cea0595..2c49954 100644
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate.c
@@ -73,20 +73,37 @@ int emul_test_cpuid(
          : "a" (leaf), "c" (subleaf));
 
     /*
-     * The emulator doesn't itself use MOVBE, so we can always run the
-     * respective tests.
+     * Some instructions and features can be emulated without specific
+     * hardware support.  These features are unconditionally reported here,
+     * for testing and fuzzing-coverage purposes.
      */
-    if ( leaf == 1 )
-        res->c |= 1U << 22;
-
-    /*
-     * The emulator doesn't itself use ADCX/ADOX/RDPID, so we can always run
-     * the respective tests.
-     */
-    if ( leaf == 7 && subleaf == 0 )
+    switch ( leaf )
     {
-        res->b |= 1U << 19;
-        res->c |= 1U << 22;
+    case 1:
+        res->c |= 1U << 22; /* MOVBE */
+        break;
+
+    case 7:
+        switch ( subleaf )
+        {
+        case 0:
+            res->b |= 1U << 11; /* rtm */
+            res->b |= 1U << 19; /* ADCX/ADOX */
+            res->b |= 1U << 20; /* STAC/CLAC */
+            res->b |= 1U << 24; /* CLWB */
+
+            res->c |= 1U << 22; /* RDPID */
+            break;
+        }
+        break;
+
+    case 0x80000001:
+        res->c |= 1U << 4; /* cr8_legacy */
+
+        if ( ctxt->vendor == X86_VENDOR_AMD )
+            res->c |= 1U << 7; /* misalignsse */
+
+        break;
     }
 
     return X86EMUL_OKAY;
-- 
2.1.4


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

[PATCH 10/10] tools/insn-fuzz: Always use x86_swint_emulate_all

[Andrew Cooper]

The swint_emulate parameter indicates how much extra work the emulator needs
to do to cover issues with certain hardware injection methods.

Using x86_swint_emulate_all opens up maximum coverage in the emulator.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 1906186..a5dbb2f 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -626,8 +626,7 @@ enum {
     HOOK_put_fpu,
     HOOK_invlpg,
     HOOK_vmfunc,
-    OPTION_swint_emulation, /* Two bits */
-    CANONICALIZE_rip = OPTION_swint_emulation + 2,
+    CANONICALIZE_rip,
     CANONICALIZE_rsp,
     CANONICALIZE_rbp
 };
@@ -669,21 +668,6 @@ static void disable_hooks(struct x86_emulate_ctxt *ctxt)
     MAYBE_DISABLE_HOOK(invlpg);
 }
 
-static void set_swint_support(struct x86_emulate_ctxt *ctxt)
-{
-    struct fuzz_state *s = ctxt->data;
-    struct fuzz_corpus *c = s->corpus;
-    unsigned int swint_opt = (c->options >> OPTION_swint_emulation) & 3;
-    static const enum x86_swint_emulation map[4] = {
-        x86_swint_emulate_none,
-        x86_swint_emulate_none,
-        x86_swint_emulate_icebp,
-        x86_swint_emulate_all
-    };
-
-    ctxt->swint_emulate = map[swint_opt];
-}
-
 /*
  * Constrain input to architecturally-possible states where
  * the emulator relies on these
@@ -762,6 +746,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
     };
     struct x86_emulate_ctxt ctxt = {
         .data = &state,
+        .swint_emulate = x86_swint_emulate_all,
         .regs = &regs,
         .addr_size = 8 * sizeof(void *),
         .sp_size = 8 * sizeof(void *),
@@ -792,8 +777,6 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
 
     disable_hooks(&ctxt);
 
-    set_swint_support(&ctxt);
-
     do {
         /* FIXME: Until we actually implement SIGFPE handling properly */
         setup_fpu_exception_handler();
-- 
2.1.4


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

Re: [PATCH 10/10] tools/insn-fuzz: Always use x86_swint_emulate_all

[George Dunlap]

On 27/03/17 10:56, Andrew Cooper wrote:
> The swint_emulate parameter indicates how much extra work the emulator needs
> to do to cover issues with certain hardware injection methods.
> 
> Using x86_swint_emulate_all opens up maximum coverage in the emulator.

Uh, no -- removing this means all of the x86_swint_emulate_none
codepaths don't get tested.

The idea here is to make sure that the emulator works for all possible
inputs.  Changing this means that there could (in theory) be a bug that
is only triggered when ctx->swint_emulate != x86_swint_emulate_all that
we wouldn't catch.

 -George


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

Re: [PATCH 03/10] tools/insn-fuzz: Don't use memcpy() for zero-length reads

[George Dunlap]

On 27/03/17 10:56, Andrew Cooper wrote:
> For control-flow changes, the emulator needs to perform a zero-length
> instruction fetch at the target offset.  It also passes NULL for the
> destination buffer, as there is no instruction stream to collect.
> 
> This trips up UBSAN, even with a size of 0.  Exclude zero-length reads from
> using memcpy(), rather than excluding NULL destination pointers, to still
> catch unintentional uses of NULL.

So memcpy() will actually try to write to dst even if bytes == 0?

That seems a bit strange, but OK:

Acked-by: George Dunlap <george.dunlap@citrix.com>

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> index 890642c..cbdb3dd 100644
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -77,7 +77,7 @@ static int data_read(const char *why, void *dst, unsigned int bytes)
>      else
>          rc = maybe_fail(why, true);
>  
> -    if ( rc == X86EMUL_OKAY )
> +    if ( rc == X86EMUL_OKAY && bytes )
>      {
>          memcpy(dst,  input.data + data_index, bytes);
>          data_index += bytes;
> 


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

Re: [PATCH 03/10] tools/insn-fuzz: Don't use memcpy() for zero-length reads

[Andrew Cooper]

On 27/03/17 12:02, George Dunlap wrote:
> On 27/03/17 10:56, Andrew Cooper wrote:
>> For control-flow changes, the emulator needs to perform a zero-length
>> instruction fetch at the target offset.  It also passes NULL for the
>> destination buffer, as there is no instruction stream to collect.
>>
>> This trips up UBSAN, even with a size of 0.  Exclude zero-length reads from
>> using memcpy(), rather than excluding NULL destination pointers, to still
>> catch unintentional uses of NULL.
> So memcpy() will actually try to write to dst even if bytes == 0?
>
> That seems a bit strange, but OK:
>
> Acked-by: George Dunlap <george.dunlap@citrix.com>

This is the undefined behaviour sanitiser, which actually objects to
passing NULL to a function annotated with
__attribute__((notnull(...))).  The check is performed before making the
call, and doesn't account for nothing happening if size is 0.

~Andrew

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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[George Dunlap]

On 27/03/17 10:56, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate.c
> index cea0595..2c49954 100644
> --- a/tools/tests/x86_emulator/x86_emulate.c
> +++ b/tools/tests/x86_emulator/x86_emulate.c
> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>           : "a" (leaf), "c" (subleaf));
>  Oh, s
>      /*
> -     * The emulator doesn't itself use MOVBE, so we can always run the
> -     * respective tests.
> +     * Some instructions and features can be emulated without specific
> +     * hardware support.  These features are unconditionally reported here,
> +     * for testing and fuzzing-coverage purposes.

But similarly to my question in patch 10 -- is there any chance that the
emulator will ever be called with a cpuid callback that returns 'false"
for these?  If so, isn't there therefore a chance that there will be
some sort of bug which only triggers if these bits are set to 'false'?

 -George



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

Re: [PATCH 01/10] x86/emul: Correct the decoding of vlddqu

[Jan Beulich]

>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> @@ -2332,9 +2333,9 @@ x86_decode_twobyte(
>          if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */
>          {
>      case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */
> -            state->desc = DstImplicit | SrcMem | Mov;
> +            state->desc = DstImplicit | SrcMem | TwoOp;

Why? This is a move after all.

> @@ -2374,11 +2375,25 @@ x86_decode_twobyte(
>      case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */
>          state->desc = DstReg | SrcMem16;
>          break;
> +
> +    case 0xf0:
> +        ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);
> +        if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */
> +        {
> +        /* fall through */
> +    case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */
> +            state->desc = DstImplicit | SrcMem | TwoOp;

I'd prefer it to be Mov here too, as the insn is a move even if its
name doesn't say so.

Jan


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

Re: [PATCH 02/10] x86/emul: Add feature check for clzero

[Jan Beulich]

>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> 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

Re: [PATCH 02/10] x86/emul: Add feature check for clzero

[Jan Beulich]

>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> @@ -5183,6 +5184,8 @@ x86_emulate(
>          {
>              unsigned long zero = 0;
>  
> +            vcpu_must_have(clzero);

Hmm, wait - doesn't this break the test harness? I.e. don't you need
to also adjust emul_test_cpuid()?

Jan


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

Re: [PATCH 03/10] tools/insn-fuzz: Don't use memcpy() for zero-length reads

[Jan Beulich]

>>> On 27.03.17 at 13:05, <andrew.cooper3@citrix.com> wrote:
> On 27/03/17 12:02, George Dunlap wrote:
>> On 27/03/17 10:56, Andrew Cooper wrote:
>>> For control-flow changes, the emulator needs to perform a zero-length
>>> instruction fetch at the target offset.  It also passes NULL for the
>>> destination buffer, as there is no instruction stream to collect.
>>>
>>> This trips up UBSAN, even with a size of 0.  Exclude zero-length reads from
>>> using memcpy(), rather than excluding NULL destination pointers, to still
>>> catch unintentional uses of NULL.
>> So memcpy() will actually try to write to dst even if bytes == 0?
>>
>> That seems a bit strange, but OK:
>>
>> Acked-by: George Dunlap <george.dunlap@citrix.com>
> 
> This is the undefined behaviour sanitiser, which actually objects to
> passing NULL to a function annotated with
> __attribute__((notnull(...))).  The check is performed before making the
> call, and doesn't account for nothing happening if size is 0.

But then why is the function annotated "nonnull"? Iirc there's
nothing in the standard disallowing NULL to be passed here so
long as length is also zero, or even only calling this undefined
behavior.

Jan


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

Re: [PATCH 03/10] tools/insn-fuzz: Don't use memcpy() for zero-length reads

[Jan Beulich]

>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> For control-flow changes, the emulator needs to perform a zero-length
> instruction fetch at the target offset.  It also passes NULL for the
> destination buffer, as there is no instruction stream to collect.
> 
> This trips up UBSAN, even with a size of 0.  Exclude zero-length reads from
> using memcpy(), rather than excluding NULL destination pointers, to still
> catch unintentional uses of NULL.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 2 +-

Btw., shouldn't this entire directory, just like tools/tests/x86_emulator/,
fall under X86 in ./MAINTAINERS?

> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -77,7 +77,7 @@ static int data_read(const char *why, void *dst, unsigned int bytes)
>      else
>          rc = maybe_fail(why, true);
>  
> -    if ( rc == X86EMUL_OKAY )
> +    if ( rc == X86EMUL_OKAY && bytes )

So if we really want to work around this oddity, then I think we want
- a comment here
- the check moved into fuzz_insn_fetch()

Jan


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

Re: [PATCH 04/10] tools/insn-fuzz: Avoid making use of static data

[Jan Beulich]

>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> AFL has a measure of stability, where it passes the same corpus into the
> fuzzing harness and observes whether the execution path changes from before.
> Any instability in the fuzzing harness reduces its effectiveness, as an
> observed crash may not reliably be caused by the original corpus.
> 
> In preparation to fix a stability bug, introduce struct fuzz_state, 
> allocated
> on the stack and passed around via struct x86_emulate_ctxt's data parameter.
> Propagate ctxt into the helpers such as maybe_fail(), so the state can be
> retrieved.
> 
> Move the previously-static data_{index,num} into struct fuzz_state.
> 
> 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

Re: [PATCH 05/10] tools/insn-fuzz: Fix a stability bug in afl-clang-fast mode

[Jan Beulich]

>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> The fuzzing harness conditionally disables hooks to test error paths in the
> emulator.  However, fuzz_emulops is a static structure.
> 
> c/s 69f4633 "tools/insn-fuzz: Support AFL's afl-clang-fast mode" introduced
> persistent mode, but because fuzz_emulops is static, the clobbering of hooks
> accumulates over repeated input, meaning that previous corpora influence the
> execution over the current corpus.
> 
> Move the partially clobbered struct x86_emulate_ops into struct fuzz_state,
> which is re-initialised from full on each call to LLVMFuzzerTestOneInput()
> 
> 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

Re: [PATCH 06/10] tools/insn-fuzz: Correct hook prototypes, and assert() appropriate segments

[Jan Beulich]

>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> The correct prototypes for the hooks are to use enum x86_segment rather than
> unsigned int.  It is implementation specific as to whether this compiles.

I'm actually surprised this has worked so far. We should fix the test
harness in the same way.

> @@ -235,27 +246,37 @@ static int fuzz_rep_stos(
>      unsigned long *reps,
>      struct x86_emulate_ctxt *ctxt)
>  {
> +    /*
> +     * STOS itself may only have an %es segment, but the stos() hook is reused
> +     * for CLZERO.
> +     */
> +    assert(is_x86_user_segment(seg));

Perhaps worth looking at ctxt->opcode?

>  static int fuzz_cmpxchg(
> -    unsigned int seg,
> +    enum x86_segment seg,
>      unsigned long offset,
>      void *old,
>      void *new,
>      unsigned int bytes,
>      struct x86_emulate_ctxt *ctxt)
>  {
> +    assert((unsigned int)seg < x86_seg_none);

I guess this could be slightly more strict, not allowing IDTR and TR.
Perhaps then also for the write handler.

Other than the above (which are only suggestions)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 07/10] tools/insn-fuzz: Provide IA32_DEBUGCTL consistently to the emulator

[Jan Beulich]

>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> x86_emulates()'s is_branch_step() performs a speculative read of
> IA32_DEBUGCTL, but doesn't squash exceptions should they arise.  In reality,
> this MSR is always available.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

While looking at this I did notice though that the use of
MSR_INDEX_MAX leads to MSR index zero
(MSR_IA32_P5_MC_ADDR) to always have a value of zero (until
all array slots would actually be used). Not actively a problem
right now, but not entirely correct either.

Jan


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

Re: [PATCH 08/10] tools/insn-fuzz: Fix assertion failures in x86_emulate_wrapper()

[Jan Beulich]

>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> c/s 92cf67888 "x86/emul: Hold x86_emulate() to strict X86EMUL_EXCEPTION
> requirements" was appropriate for the hypervisor, but the fuzzer stubs didn't
> conform to the stricter requirements.  AFL is very quick to discover this.
> 
> Extend the fuzzing harness exception logic to raise exceptions 
> appropriately.
> 
> 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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[Jan Beulich]

>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/tests/x86_emulator/x86_emulate.c | 41 
> ++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/tests/x86_emulator/x86_emulate.c 
> b/tools/tests/x86_emulator/x86_emulate.c
> index cea0595..2c49954 100644
> --- a/tools/tests/x86_emulator/x86_emulate.c
> +++ b/tools/tests/x86_emulator/x86_emulate.c
> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>           : "a" (leaf), "c" (subleaf));
>  
>      /*
> -     * The emulator doesn't itself use MOVBE, so we can always run the
> -     * respective tests.
> +     * Some instructions and features can be emulated without specific
> +     * hardware support.  These features are unconditionally reported here,
> +     * for testing and fuzzing-coverage purposes.
>       */
> -    if ( leaf == 1 )
> -        res->c |= 1U << 22;
> -
> -    /*
> -     * The emulator doesn't itself use ADCX/ADOX/RDPID, so we can always 
> run
> -     * the respective tests.
> -     */
> -    if ( leaf == 7 && subleaf == 0 )
> +    switch ( leaf )
>      {
> -        res->b |= 1U << 19;
> -        res->c |= 1U << 22;
> +    case 1:
> +        res->c |= 1U << 22; /* MOVBE */
> +        break;
> +
> +    case 7:
> +        switch ( subleaf )
> +        {
> +        case 0:
> +            res->b |= 1U << 11; /* rtm */

Upper case?

> +            res->b |= 1U << 19; /* ADCX/ADOX */
> +            res->b |= 1U << 20; /* STAC/CLAC */

SMAP?

> +            res->b |= 1U << 24; /* CLWB */
> +
> +            res->c |= 1U << 22; /* RDPID */
> +            break;
> +        }
> +        break;
> +
> +    case 0x80000001:
> +        res->c |= 1U << 4; /* cr8_legacy */

I think this one is AMD-only, just like ...

> +        if ( ctxt->vendor == X86_VENDOR_AMD )
> +            res->c |= 1U << 7; /* misalignsse */

... this.

And what about LAHF_LM, LZCNT, and CLFLUSH{,OPT}?

Jan


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

Re: [PATCH 01/10] x86/emul: Correct the decoding of vlddqu

[Andrew Cooper]

On 27/03/17 12:24, Jan Beulich wrote:
>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>> @@ -2332,9 +2333,9 @@ x86_decode_twobyte(
>>          if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */
>>          {
>>      case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */
>> -            state->desc = DstImplicit | SrcMem | Mov;
>> +            state->desc = DstImplicit | SrcMem | TwoOp;
> Why? This is a move after all.
>
>> @@ -2374,11 +2375,25 @@ x86_decode_twobyte(
>>      case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */
>>          state->desc = DstReg | SrcMem16;
>>          break;
>> +
>> +    case 0xf0:
>> +        ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);
>> +        if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */
>> +        {
>> +        /* fall through */
>> +    case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */
>> +            state->desc = DstImplicit | SrcMem | TwoOp;
> I'd prefer it to be Mov here too, as the insn is a move even if its
> name doesn't say so.

The fact that TwoOp and Mov are the same constant is confusing (and why
I didn't particularly like its introduction in the first place),
especially in this context where TwoOp is the more important semantic
piece of information.

~Andrew

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

Re: [PATCH 02/10] x86/emul: Add feature check for clzero

[Andrew Cooper]

On 27/03/17 12:28, Jan Beulich wrote:
>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>> @@ -5183,6 +5184,8 @@ x86_emulate(
>>          {
>>              unsigned long zero = 0;
>>  
>> +            vcpu_must_have(clzero);
> Hmm, wait - doesn't this break the test harness? I.e. don't you need
> to also adjust emul_test_cpuid()?

The logical chain of events which lead to this discovery was an
assertion failure in the stos() hook, and wondering how I hit it fuzzing
on an Intel machine.

The emul_test_cpuid() adjustment is later in the series with other
related changes.

~Andrew

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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[Jan Beulich]

>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote:
> On 27/03/17 10:56, Andrew Cooper wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++----------
>>  1 file changed, 29 insertions(+), 12 deletions(-)
>> 
>> diff --git a/tools/tests/x86_emulator/x86_emulate.c 
> b/tools/tests/x86_emulator/x86_emulate.c
>> index cea0595..2c49954 100644
>> --- a/tools/tests/x86_emulator/x86_emulate.c
>> +++ b/tools/tests/x86_emulator/x86_emulate.c
>> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>>           : "a" (leaf), "c" (subleaf));
>>  Oh, s
>>      /*
>> -     * The emulator doesn't itself use MOVBE, so we can always run the
>> -     * respective tests.
>> +     * Some instructions and features can be emulated without specific
>> +     * hardware support.  These features are unconditionally reported here,
>> +     * for testing and fuzzing-coverage purposes.
> 
> But similarly to my question in patch 10 -- is there any chance that the
> emulator will ever be called with a cpuid callback that returns 'false"
> for these?  If so, isn't there therefore a chance that there will be
> some sort of bug which only triggers if these bits are set to 'false'?

I think I've suggested before that the cpuid hook should actually
return void, as it can't possibly fail (now that CPUID faulting is
being handled in generic code).

Jan


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

Re: [PATCH 03/10] tools/insn-fuzz: Don't use memcpy() for zero-length reads

[Andrew Cooper]

On 27/03/17 12:36, Jan Beulich wrote:
>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>> For control-flow changes, the emulator needs to perform a zero-length
>> instruction fetch at the target offset.  It also passes NULL for the
>> destination buffer, as there is no instruction stream to collect.
>>
>> This trips up UBSAN, even with a size of 0.  Exclude zero-length reads from
>> using memcpy(), rather than excluding NULL destination pointers, to still
>> catch unintentional uses of NULL.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 2 +-
> Btw., shouldn't this entire directory, just like tools/tests/x86_emulator/,
> fall under X86 in ./MAINTAINERS?

Yes probably.  I will adjust that in v2.

>
>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> @@ -77,7 +77,7 @@ static int data_read(const char *why, void *dst, unsigned int bytes)
>>      else
>>          rc = maybe_fail(why, true);
>>  
>> -    if ( rc == X86EMUL_OKAY )
>> +    if ( rc == X86EMUL_OKAY && bytes )
> So if we really want to work around this oddity, then I think we want
> - a comment here
> - the check moved into fuzz_insn_fetch()

Ok.

~Andrew

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

Re: [PATCH 03/10] tools/insn-fuzz: Don't use memcpy() for zero-length reads

[Andrew Cooper]

On 27/03/17 12:32, Jan Beulich wrote:
>>>> On 27.03.17 at 13:05, <andrew.cooper3@citrix.com> wrote:
>> On 27/03/17 12:02, George Dunlap wrote:
>>> On 27/03/17 10:56, Andrew Cooper wrote:
>>>> For control-flow changes, the emulator needs to perform a zero-length
>>>> instruction fetch at the target offset.  It also passes NULL for the
>>>> destination buffer, as there is no instruction stream to collect.
>>>>
>>>> This trips up UBSAN, even with a size of 0.  Exclude zero-length reads from
>>>> using memcpy(), rather than excluding NULL destination pointers, to still
>>>> catch unintentional uses of NULL.
>>> So memcpy() will actually try to write to dst even if bytes == 0?
>>>
>>> That seems a bit strange, but OK:
>>>
>>> Acked-by: George Dunlap <george.dunlap@citrix.com>
>> This is the undefined behaviour sanitiser, which actually objects to
>> passing NULL to a function annotated with
>> __attribute__((notnull(...))).  The check is performed before making the
>> call, and doesn't account for nothing happening if size is 0.
> But then why is the function annotated "nonnull"? Iirc there's
> nothing in the standard disallowing NULL to be passed here so
> long as length is also zero, or even only calling this undefined
> behavior.

From n1570,

7.24.1 "String function conventions"

"Where an argument declared as size_t n specifies the length of the
array for a function, n can have the value zero on a call to that
function. Unless explicitly stated otherwise in the description of a
particular function in this subclause, pointer arguments on such a call
shall still have valid values, as described in 7.1.4. On such a call, a
function that locates a character finds no occurrence, a function that
compares two character sequences returns zero, and a function that
copies characters copies zero characters."

And from 7.1.4: "Use of library functions"

"If an argument to a function has an invalid value (such as a value
outside the domain of the function, or a pointer outside the address
space of the program, or a null pointer, or a pointer to non-modifiable
storage when the corresponding parameter is not const-qualified) or a
type (after promotion) not expected by a function with variable number
of arguments, the behavior is undefined."


Therefore, by my reading, the nonnull attribute is correct.

~Andrew

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

Re: [PATCH 01/10] x86/emul: Correct the decoding of vlddqu

[Jan Beulich]

>>> On 27.03.17 at 14:10, <andrew.cooper3@citrix.com> wrote:
> On 27/03/17 12:24, Jan Beulich wrote:
>>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>>> @@ -2332,9 +2333,9 @@ x86_decode_twobyte(
>>>          if ( vex.pfx == vex_f3 ) /* movq xmm/m64,xmm */
>>>          {
>>>      case X86EMUL_OPC_VEX_F3(0, 0x7e): /* vmovq xmm/m64,xmm */
>>> -            state->desc = DstImplicit | SrcMem | Mov;
>>> +            state->desc = DstImplicit | SrcMem | TwoOp;
>> Why? This is a move after all.
>>
>>> @@ -2374,11 +2375,25 @@ x86_decode_twobyte(
>>>      case X86EMUL_OPC_VEX_66(0, 0xc4): /* vpinsrw */
>>>          state->desc = DstReg | SrcMem16;
>>>          break;
>>> +
>>> +    case 0xf0:
>>> +        ctxt->opcode |= MASK_INSR(vex.pfx, X86EMUL_OPC_PFX_MASK);
>>> +        if ( vex.pfx == vex_f2 ) /* lddqu mem,xmm */
>>> +        {
>>> +        /* fall through */
>>> +    case X86EMUL_OPC_VEX_F2(0, 0xf0): /* vlddqu mem,{x,y}mm */
>>> +            state->desc = DstImplicit | SrcMem | TwoOp;
>> I'd prefer it to be Mov here too, as the insn is a move even if its
>> name doesn't say so.
> 
> The fact that TwoOp and Mov are the same constant is confusing (and why
> I didn't particularly like its introduction in the first place),
> especially in this context where TwoOp is the more important semantic
> piece of information.

Well, okay, if we consider that there aren't any real RMW SIMD
insns (albeit for now we emulate a few as such), I can buy this
argument, so

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

Jan


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

Re: [PATCH 02/10] x86/emul: Add feature check for clzero

[Jan Beulich]

>>> On 27.03.17 at 14:13, <andrew.cooper3@citrix.com> wrote:
> On 27/03/17 12:28, Jan Beulich wrote:
>>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>>> @@ -5183,6 +5184,8 @@ x86_emulate(
>>>          {
>>>              unsigned long zero = 0;
>>>  
>>> +            vcpu_must_have(clzero);
>> Hmm, wait - doesn't this break the test harness? I.e. don't you need
>> to also adjust emul_test_cpuid()?
> 
> The logical chain of events which lead to this discovery was an
> assertion failure in the stos() hook, and wondering how I hit it fuzzing
> on an Intel machine.
> 
> The emul_test_cpuid() adjustment is later in the series with other
> related changes.

I don't think I've seen CLZERO there, but the (initial) adjustment
would belong here anyway.

Jan


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

Re: [PATCH 03/10] tools/insn-fuzz: Don't use memcpy() for zero-length reads

[Jan Beulich]

>>> On 27.03.17 at 14:22, <andrew.cooper3@citrix.com> wrote:
> On 27/03/17 12:32, Jan Beulich wrote:
>>>>> On 27.03.17 at 13:05, <andrew.cooper3@citrix.com> wrote:
>>> On 27/03/17 12:02, George Dunlap wrote:
>>>> On 27/03/17 10:56, Andrew Cooper wrote:
>>>>> For control-flow changes, the emulator needs to perform a zero-length
>>>>> instruction fetch at the target offset.  It also passes NULL for the
>>>>> destination buffer, as there is no instruction stream to collect.
>>>>>
>>>>> This trips up UBSAN, even with a size of 0.  Exclude zero-length reads from
>>>>> using memcpy(), rather than excluding NULL destination pointers, to still
>>>>> catch unintentional uses of NULL.
>>>> So memcpy() will actually try to write to dst even if bytes == 0?
>>>>
>>>> That seems a bit strange, but OK:
>>>>
>>>> Acked-by: George Dunlap <george.dunlap@citrix.com>
>>> This is the undefined behaviour sanitiser, which actually objects to
>>> passing NULL to a function annotated with
>>> __attribute__((notnull(...))).  The check is performed before making the
>>> call, and doesn't account for nothing happening if size is 0.
>> But then why is the function annotated "nonnull"? Iirc there's
>> nothing in the standard disallowing NULL to be passed here so
>> long as length is also zero, or even only calling this undefined
>> behavior.
> 
> From n1570,
> 
> 7.24.1 "String function conventions"
> 
> "Where an argument declared as size_t n specifies the length of the
> array for a function, n can have the value zero on a call to that
> function. Unless explicitly stated otherwise in the description of a
> particular function in this subclause, pointer arguments on such a call
> shall still have valid values, as described in 7.1.4. On such a call, a
> function that locates a character finds no occurrence, a function that
> compares two character sequences returns zero, and a function that
> copies characters copies zero characters."
> 
> And from 7.1.4: "Use of library functions"
> 
> "If an argument to a function has an invalid value (such as a value
> outside the domain of the function, or a pointer outside the address
> space of the program, or a null pointer, or a pointer to non-modifiable
> storage when the corresponding parameter is not const-qualified) or a
> type (after promotion) not expected by a function with variable number
> of arguments, the behavior is undefined."
> 
> 
> Therefore, by my reading, the nonnull attribute is correct.

Hmm, indeed. I continue to be surprised by how many unnecessary
restrictions the standard sets up. With the above, I'm afraid we have
a number of undefined function calls in our tree.

Jan


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

Re: [PATCH 06/10] tools/insn-fuzz: Correct hook prototypes, and assert() appropriate segments

[Andrew Cooper]

On 27/03/17 12:48, Jan Beulich wrote:
>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>> The correct prototypes for the hooks are to use enum x86_segment rather than
>> unsigned int.  It is implementation specific as to whether this compiles.
> I'm actually surprised this has worked so far. We should fix the test
> harness in the same way.

Oh yes.  I will fix that as well.

>
>> @@ -235,27 +246,37 @@ static int fuzz_rep_stos(
>>      unsigned long *reps,
>>      struct x86_emulate_ctxt *ctxt)
>>  {
>> +    /*
>> +     * STOS itself may only have an %es segment, but the stos() hook is reused
>> +     * for CLZERO.
>> +     */
>> +    assert(is_x86_user_segment(seg));
> Perhaps worth looking at ctxt->opcode?

I considered that but chose not to.  I think starting to special case
like that might get unwieldy.

>
>>  static int fuzz_cmpxchg(
>> -    unsigned int seg,
>> +    enum x86_segment seg,
>>      unsigned long offset,
>>      void *old,
>>      void *new,
>>      unsigned int bytes,
>>      struct x86_emulate_ctxt *ctxt)
>>  {
>> +    assert((unsigned int)seg < x86_seg_none);
> I guess this could be slightly more strict, not allowing IDTR and TR.
> Perhaps then also for the write handler.

Hmm - good point.  We have no architectural reason to perform a
cmpxchg() targeting the IDT or TR.

OTOH, we have no architectural reason to ever write to any of the system
segments, so that side of things can be stricter.

~Andrew

>
> Other than the above (which are only suggestions)
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Jan
>


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

Re: [PATCH 07/10] tools/insn-fuzz: Provide IA32_DEBUGCTL consistently to the emulator

[Andrew Cooper]

On 27/03/17 12:53, Jan Beulich wrote:
>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>> x86_emulates()'s is_branch_step() performs a speculative read of
>> IA32_DEBUGCTL, but doesn't squash exceptions should they arise.  In reality,
>> this MSR is always available.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

> While looking at this I did notice though that the use of
> MSR_INDEX_MAX leads to MSR index zero
> (MSR_IA32_P5_MC_ADDR) to always have a value of zero (until
> all array slots would actually be used). Not actively a problem
> right now, but not entirely correct either.

I have some plans to entirely rework the MSR/CR handing in the fuzzing
harness.  At the moment, AFL is wasting a lot of effort mutating large
areas of the input corpus to try and find new paths, to no avail.

This change is the minimum required to satisfy the existing assertions.

~Andrew

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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[George Dunlap]

On 27/03/17 13:13, Jan Beulich wrote:
>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote:
>> On 27/03/17 10:56, Andrew Cooper wrote:
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>>  tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++----------
>>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c 
>> b/tools/tests/x86_emulator/x86_emulate.c
>>> index cea0595..2c49954 100644
>>> --- a/tools/tests/x86_emulator/x86_emulate.c
>>> +++ b/tools/tests/x86_emulator/x86_emulate.c
>>> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>>>           : "a" (leaf), "c" (subleaf));
>>>  Oh, s
>>>      /*
>>> -     * The emulator doesn't itself use MOVBE, so we can always run the
>>> -     * respective tests.
>>> +     * Some instructions and features can be emulated without specific
>>> +     * hardware support.  These features are unconditionally reported here,
>>> +     * for testing and fuzzing-coverage purposes.
>>
>> But similarly to my question in patch 10 -- is there any chance that the
>> emulator will ever be called with a cpuid callback that returns 'false"
>> for these?  If so, isn't there therefore a chance that there will be
>> some sort of bug which only triggers if these bits are set to 'false'?
> 
> I think I've suggested before that the cpuid hook should actually
> return void, as it can't possibly fail (now that CPUID faulting is
> being handled in generic code).

This isn't about failing so much as it is about reporting the presence /
absence of hardware features.  With this patch, cpuid unconditionally
advertises the presence of a number of features (MOVBE, rtm, ADCX/ADOX,
&c) because the emulation will work even if the features aren't actually
present in hardware.  I'm suggesting that we may want to make sure that
we test *both* the "feature is present" path, *and* the "feature is
missing" path.

 -George


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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[Andrew Cooper]

On 27/03/17 13:09, Jan Beulich wrote:
>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> ---
>>  tools/tests/x86_emulator/x86_emulate.c | 41 
>> ++++++++++++++++++++++++----------
>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/tests/x86_emulator/x86_emulate.c 
>> b/tools/tests/x86_emulator/x86_emulate.c
>> index cea0595..2c49954 100644
>> --- a/tools/tests/x86_emulator/x86_emulate.c
>> +++ b/tools/tests/x86_emulator/x86_emulate.c
>> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>>           : "a" (leaf), "c" (subleaf));
>>  
>>      /*
>> -     * The emulator doesn't itself use MOVBE, so we can always run the
>> -     * respective tests.
>> +     * Some instructions and features can be emulated without specific
>> +     * hardware support.  These features are unconditionally reported here,
>> +     * for testing and fuzzing-coverage purposes.
>>       */
>> -    if ( leaf == 1 )
>> -        res->c |= 1U << 22;
>> -
>> -    /*
>> -     * The emulator doesn't itself use ADCX/ADOX/RDPID, so we can always 
>> run
>> -     * the respective tests.
>> -     */
>> -    if ( leaf == 7 && subleaf == 0 )
>> +    switch ( leaf )
>>      {
>> -        res->b |= 1U << 19;
>> -        res->c |= 1U << 22;
>> +    case 1:
>> +        res->c |= 1U << 22; /* MOVBE */
>> +        break;
>> +
>> +    case 7:
>> +        switch ( subleaf )
>> +        {
>> +        case 0:
>> +            res->b |= 1U << 11; /* rtm */
> Upper case?

I was trying to visually separate the instructions from the features.

>
>> +            res->b |= 1U << 19; /* ADCX/ADOX */
>> +            res->b |= 1U << 20; /* STAC/CLAC */
> SMAP?
>
>> +            res->b |= 1U << 24; /* CLWB */
>> +
>> +            res->c |= 1U << 22; /* RDPID */
>> +            break;
>> +        }
>> +        break;
>> +
>> +    case 0x80000001:
>> +        res->c |= 1U << 4; /* cr8_legacy */
> I think this one is AMD-only, just like ...
>
>> +        if ( ctxt->vendor == X86_VENDOR_AMD )
>> +            res->c |= 1U << 7; /* misalignsse */
> ... this.

The difference is that cr8_legacy is a straight "is this specific
encoding valid to use", while misalignsse causes rather larger changes
in how SSE arguments get handled.

> And what about LAHF_LM, LZCNT, and CLFLUSH{,OPT}?

This list was only the instructions I had encountered.  I can add all of
these.

~Andrew

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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[Andrew Cooper]

On 27/03/17 13:56, George Dunlap wrote:
> On 27/03/17 13:13, Jan Beulich wrote:
>>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote:
>>> On 27/03/17 10:56, Andrew Cooper wrote:
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>> ---
>>>>  tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++----------
>>>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c 
>>> b/tools/tests/x86_emulator/x86_emulate.c
>>>> index cea0595..2c49954 100644
>>>> --- a/tools/tests/x86_emulator/x86_emulate.c
>>>> +++ b/tools/tests/x86_emulator/x86_emulate.c
>>>> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>>>>           : "a" (leaf), "c" (subleaf));
>>>>  Oh, s
>>>>      /*
>>>> -     * The emulator doesn't itself use MOVBE, so we can always run the
>>>> -     * respective tests.
>>>> +     * Some instructions and features can be emulated without specific
>>>> +     * hardware support.  These features are unconditionally reported here,
>>>> +     * for testing and fuzzing-coverage purposes.
>>> But similarly to my question in patch 10 -- is there any chance that the
>>> emulator will ever be called with a cpuid callback that returns 'false"
>>> for these?  If so, isn't there therefore a chance that there will be
>>> some sort of bug which only triggers if these bits are set to 'false'?
>> I think I've suggested before that the cpuid hook should actually
>> return void, as it can't possibly fail (now that CPUID faulting is
>> being handled in generic code).
> This isn't about failing so much as it is about reporting the presence /
> absence of hardware features.  With this patch, cpuid unconditionally
> advertises the presence of a number of features (MOVBE, rtm, ADCX/ADOX,
> &c) because the emulation will work even if the features aren't actually
> present in hardware.  I'm suggesting that we may want to make sure that
> we test *both* the "feature is present" path, *and* the "feature is
> missing" path.

I have some plans to make this happen, but it isn't easy with the
existing infrastructure.  In the meantime, It is more important to get
better coverage.

~Andrew

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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[George Dunlap]

On 27/03/17 14:03, Andrew Cooper wrote:
> On 27/03/17 13:56, George Dunlap wrote:
>> On 27/03/17 13:13, Jan Beulich wrote:
>>>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote:
>>>> On 27/03/17 10:56, Andrew Cooper wrote:
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> ---
>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>>> ---
>>>>>  tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++----------
>>>>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c 
>>>> b/tools/tests/x86_emulator/x86_emulate.c
>>>>> index cea0595..2c49954 100644
>>>>> --- a/tools/tests/x86_emulator/x86_emulate.c
>>>>> +++ b/tools/tests/x86_emulator/x86_emulate.c
>>>>> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>>>>>           : "a" (leaf), "c" (subleaf));
>>>>>  Oh, s
>>>>>      /*
>>>>> -     * The emulator doesn't itself use MOVBE, so we can always run the
>>>>> -     * respective tests.
>>>>> +     * Some instructions and features can be emulated without specific
>>>>> +     * hardware support.  These features are unconditionally reported here,
>>>>> +     * for testing and fuzzing-coverage purposes.
>>>> But similarly to my question in patch 10 -- is there any chance that the
>>>> emulator will ever be called with a cpuid callback that returns 'false"
>>>> for these?  If so, isn't there therefore a chance that there will be
>>>> some sort of bug which only triggers if these bits are set to 'false'?
>>> I think I've suggested before that the cpuid hook should actually
>>> return void, as it can't possibly fail (now that CPUID faulting is
>>> being handled in generic code).
>> This isn't about failing so much as it is about reporting the presence /
>> absence of hardware features.  With this patch, cpuid unconditionally
>> advertises the presence of a number of features (MOVBE, rtm, ADCX/ADOX,
>> &c) because the emulation will work even if the features aren't actually
>> present in hardware.  I'm suggesting that we may want to make sure that
>> we test *both* the "feature is present" path, *and* the "feature is
>> missing" path.
> 
> I have some plans to make this happen, but it isn't easy with the
> existing infrastructure.  In the meantime, It is more important to get
> better coverage.

That sounds reasonable.

Acked-by: George Dunlap <george.dunlap@citrix.com>


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

Re: [PATCH 10/10] tools/insn-fuzz: Always use x86_swint_emulate_all

[Andrew Cooper]

On 27/03/17 12:00, George Dunlap wrote:
> On 27/03/17 10:56, Andrew Cooper wrote:
>> The swint_emulate parameter indicates how much extra work the emulator needs
>> to do to cover issues with certain hardware injection methods.
>>
>> Using x86_swint_emulate_all opens up maximum coverage in the emulator.
> Uh, no -- removing this means all of the x86_swint_emulate_none
> codepaths don't get tested.

Which codepaths are these?

>
> The idea here is to make sure that the emulator works for all possible
> inputs.  Changing this means that there could (in theory) be a bug that
> is only triggered when ctx->swint_emulate != x86_swint_emulate_all that
> we wouldn't catch.

swint_emulate isn't a regular input.  The only thing it gates is whether
we do work in inject_swint() or not, and it only exists because of SVM's
inability to correctly inject certain events.  (In fact, the more I
think about it, the more I think it ought to move into the svm code
rather than polluting the common emulator.)

This property is not going to change, and coverage inside inject_swint()
is far more important from a fuzzing point of view.

~Andrew

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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[Andrew Cooper]

On 27/03/17 13:13, Jan Beulich wrote:
>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote:
>> On 27/03/17 10:56, Andrew Cooper wrote:
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> ---
>>>  tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++----------
>>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c 
>> b/tools/tests/x86_emulator/x86_emulate.c
>>> index cea0595..2c49954 100644
>>> --- a/tools/tests/x86_emulator/x86_emulate.c
>>> +++ b/tools/tests/x86_emulator/x86_emulate.c
>>> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>>>           : "a" (leaf), "c" (subleaf));
>>>  Oh, s
>>>      /*
>>> -     * The emulator doesn't itself use MOVBE, so we can always run the
>>> -     * respective tests.
>>> +     * Some instructions and features can be emulated without specific
>>> +     * hardware support.  These features are unconditionally reported here,
>>> +     * for testing and fuzzing-coverage purposes.
>> But similarly to my question in patch 10 -- is there any chance that the
>> emulator will ever be called with a cpuid callback that returns 'false"
>> for these?  If so, isn't there therefore a chance that there will be
>> some sort of bug which only triggers if these bits are set to 'false'?
> I think I've suggested before that the cpuid hook should actually
> return void, as it can't possibly fail (now that CPUID faulting is
> being handled in generic code).

I've been considering this quite a lot recently.  One the one hand, the
introspection hook for CPUID really ought to be using X86EMUL_RETRY.

On the other, we really are (ab)using the existing cpuid() hook for two
different purposes.  There really is a conceptual difference between
issuing a cpuid() call as part of emulating a CPUID instruction, and
using it to find out whether other instructions are permitted.  The
latter is synonymous to having or not having the requisite piece of
silicon, and isn't something which can fail.

In light of the new struct cpuid_policy, I was considering exposing that
to the emulator, so the feature checks are straight bit tests.  This
would separate the two different purposes we have, and should reduce the
size of x86_emulate() a little  (At the moment, it is 1/4 MB compiled,
or about 1/6th of the entire hypervisor.)

Furthermore, the soon-to-appear struct msr_policy would help resolve our
speculative MSR read issues in a similar way.

~Andrew

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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[Jan Beulich]

>>> On 27.03.17 at 15:01, <andrew.cooper3@citrix.com> wrote:
> On 27/03/17 13:09, Jan Beulich wrote:
>>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>>> --- a/tools/tests/x86_emulator/x86_emulate.c
>>> +++ b/tools/tests/x86_emulator/x86_emulate.c
>>> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>>>           : "a" (leaf), "c" (subleaf));
>>>  
>>>      /*
>>> -     * The emulator doesn't itself use MOVBE, so we can always run the
>>> -     * respective tests.
>>> +     * Some instructions and features can be emulated without specific
>>> +     * hardware support.  These features are unconditionally reported here,
>>> +     * for testing and fuzzing-coverage purposes.
>>>       */
>>> -    if ( leaf == 1 )
>>> -        res->c |= 1U << 22;
>>> -
>>> -    /*
>>> -     * The emulator doesn't itself use ADCX/ADOX/RDPID, so we can always 
>>> run
>>> -     * the respective tests.
>>> -     */
>>> -    if ( leaf == 7 && subleaf == 0 )
>>> +    switch ( leaf )
>>>      {
>>> -        res->b |= 1U << 19;
>>> -        res->c |= 1U << 22;
>>> +    case 1:
>>> +        res->c |= 1U << 22; /* MOVBE */
>>> +        break;
>>> +
>>> +    case 7:
>>> +        switch ( subleaf )
>>> +        {
>>> +        case 0:
>>> +            res->b |= 1U << 11; /* rtm */
>> Upper case?
> 
> I was trying to visually separate the instructions from the features.

Oh, I see. Fine with me then, i.e. I'd adjust ...

>>> +            res->b |= 1U << 19; /* ADCX/ADOX */
>>> +            res->b |= 1U << 20; /* STAC/CLAC */
>> SMAP?

... this to "smap?"

>>> +            res->b |= 1U << 24; /* CLWB */
>>> +
>>> +            res->c |= 1U << 22; /* RDPID */
>>> +            break;
>>> +        }
>>> +        break;
>>> +
>>> +    case 0x80000001:
>>> +        res->c |= 1U << 4; /* cr8_legacy */
>> I think this one is AMD-only, just like ...
>>
>>> +        if ( ctxt->vendor == X86_VENDOR_AMD )
>>> +            res->c |= 1U << 7; /* misalignsse */
>> ... this.
> 
> The difference is that cr8_legacy is a straight "is this specific
> encoding valid to use", while misalignsse causes rather larger changes
> in how SSE arguments get handled.

True.

>> And what about LAHF_LM, LZCNT, and CLFLUSH{,OPT}?
> 
> This list was only the instructions I had encountered.  I can add all of
> these.

Yes please (subject to the other sub-thread).

Jan


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

Re: [PATCH 02/10] x86/emul: Add feature check for clzero

[Andrew Cooper]

On 27/03/17 13:31, Jan Beulich wrote:
>>>> On 27.03.17 at 14:13, <andrew.cooper3@citrix.com> wrote:
>> On 27/03/17 12:28, Jan Beulich wrote:
>>>>>> On 27.03.17 at 11:56, <andrew.cooper3@citrix.com> wrote:
>>>> @@ -5183,6 +5184,8 @@ x86_emulate(
>>>>          {
>>>>              unsigned long zero = 0;
>>>>  
>>>> +            vcpu_must_have(clzero);
>>> Hmm, wait - doesn't this break the test harness? I.e. don't you need
>>> to also adjust emul_test_cpuid()?
>> The logical chain of events which lead to this discovery was an
>> assertion failure in the stos() hook, and wondering how I hit it fuzzing
>> on an Intel machine.
>>
>> The emul_test_cpuid() adjustment is later in the series with other
>> related changes.
> I don't think I've seen CLZERO there, but the (initial) adjustment
> would belong here anyway.

That is a very good point... I definitely remember putting it in, but I
can't find it anywhere in my git tree.  I must have lost it in a rebase
somewhere.

~Andrew

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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[Jan Beulich]

>>> On 27.03.17 at 15:03, <andrew.cooper3@citrix.com> wrote:
> On 27/03/17 13:56, George Dunlap wrote:
>> On 27/03/17 13:13, Jan Beulich wrote:
>>>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote:
>>>> On 27/03/17 10:56, Andrew Cooper wrote:
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> ---
>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>>> ---
>>>>>  tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++----------
>>>>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c 
>>>> b/tools/tests/x86_emulator/x86_emulate.c
>>>>> index cea0595..2c49954 100644
>>>>> --- a/tools/tests/x86_emulator/x86_emulate.c
>>>>> +++ b/tools/tests/x86_emulator/x86_emulate.c
>>>>> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>>>>>           : "a" (leaf), "c" (subleaf));
>>>>>  Oh, s
>>>>>      /*
>>>>> -     * The emulator doesn't itself use MOVBE, so we can always run the
>>>>> -     * respective tests.
>>>>> +     * Some instructions and features can be emulated without specific
>>>>> +     * hardware support.  These features are unconditionally reported here,
>>>>> +     * for testing and fuzzing-coverage purposes.
>>>> But similarly to my question in patch 10 -- is there any chance that the
>>>> emulator will ever be called with a cpuid callback that returns 'false"
>>>> for these?  If so, isn't there therefore a chance that there will be
>>>> some sort of bug which only triggers if these bits are set to 'false'?
>>> I think I've suggested before that the cpuid hook should actually
>>> return void, as it can't possibly fail (now that CPUID faulting is
>>> being handled in generic code).
>> This isn't about failing so much as it is about reporting the presence /
>> absence of hardware features.  With this patch, cpuid unconditionally
>> advertises the presence of a number of features (MOVBE, rtm, ADCX/ADOX,
>> &c) because the emulation will work even if the features aren't actually
>> present in hardware.  I'm suggesting that we may want to make sure that
>> we test *both* the "feature is present" path, *and* the "feature is
>> missing" path.
> 
> I have some plans to make this happen, but it isn't easy with the
> existing infrastructure.  In the meantime, It is more important to get
> better coverage.

Can't we simply grab enough bits of input data to cover the ones
of interest here, store them away, and use that instead of the
hard coded 1s here?

Jan


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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[Jan Beulich]

>>> On 27.03.17 at 15:37, <andrew.cooper3@citrix.com> wrote:
> On 27/03/17 13:13, Jan Beulich wrote:
>>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote:
>>> On 27/03/17 10:56, Andrew Cooper wrote:
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>> ---
>>>>  tools/tests/x86_emulator/x86_emulate.c | 41 
> ++++++++++++++++++++++++----------
>>>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c 
>>> b/tools/tests/x86_emulator/x86_emulate.c
>>>> index cea0595..2c49954 100644
>>>> --- a/tools/tests/x86_emulator/x86_emulate.c
>>>> +++ b/tools/tests/x86_emulator/x86_emulate.c
>>>> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>>>>           : "a" (leaf), "c" (subleaf));
>>>>  Oh, s
>>>>      /*
>>>> -     * The emulator doesn't itself use MOVBE, so we can always run the
>>>> -     * respective tests.
>>>> +     * Some instructions and features can be emulated without specific
>>>> +     * hardware support.  These features are unconditionally reported here,
>>>> +     * for testing and fuzzing-coverage purposes.
>>> But similarly to my question in patch 10 -- is there any chance that the
>>> emulator will ever be called with a cpuid callback that returns 'false"
>>> for these?  If so, isn't there therefore a chance that there will be
>>> some sort of bug which only triggers if these bits are set to 'false'?
>> I think I've suggested before that the cpuid hook should actually
>> return void, as it can't possibly fail (now that CPUID faulting is
>> being handled in generic code).
> 
> I've been considering this quite a lot recently.  One the one hand, the
> introspection hook for CPUID really ought to be using X86EMUL_RETRY.
> 
> On the other, we really are (ab)using the existing cpuid() hook for two
> different purposes.  There really is a conceptual difference between
> issuing a cpuid() call as part of emulating a CPUID instruction, and
> using it to find out whether other instructions are permitted.  The
> latter is synonymous to having or not having the requisite piece of
> silicon, and isn't something which can fail.

There may be a semantic difference, but a conceptual one? CPUID
insns can't fail either (with CPUID faulting out of the picture).

Jan


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

Re: [PATCH 09/10] tools/x86emul: Advertise more CPUID features for testing purposes

[Andrew Cooper]

On 27/03/17 14:42, Jan Beulich wrote:
>>>> On 27.03.17 at 15:03, <andrew.cooper3@citrix.com> wrote:
>> On 27/03/17 13:56, George Dunlap wrote:
>>> On 27/03/17 13:13, Jan Beulich wrote:
>>>>>>> On 27.03.17 at 13:20, <george.dunlap@citrix.com> wrote:
>>>>> On 27/03/17 10:56, Andrew Cooper wrote:
>>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> ---
>>>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>>>> CC: George Dunlap <george.dunlap@eu.citrix.com>
>>>>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>>>> ---
>>>>>>  tools/tests/x86_emulator/x86_emulate.c | 41 ++++++++++++++++++++++++----------
>>>>>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/tests/x86_emulator/x86_emulate.c 
>>>>> b/tools/tests/x86_emulator/x86_emulate.c
>>>>>> index cea0595..2c49954 100644
>>>>>> --- a/tools/tests/x86_emulator/x86_emulate.c
>>>>>> +++ b/tools/tests/x86_emulator/x86_emulate.c
>>>>>> @@ -73,20 +73,37 @@ int emul_test_cpuid(
>>>>>>           : "a" (leaf), "c" (subleaf));
>>>>>>  Oh, s
>>>>>>      /*
>>>>>> -     * The emulator doesn't itself use MOVBE, so we can always run the
>>>>>> -     * respective tests.
>>>>>> +     * Some instructions and features can be emulated without specific
>>>>>> +     * hardware support.  These features are unconditionally reported here,
>>>>>> +     * for testing and fuzzing-coverage purposes.
>>>>> But similarly to my question in patch 10 -- is there any chance that the
>>>>> emulator will ever be called with a cpuid callback that returns 'false"
>>>>> for these?  If so, isn't there therefore a chance that there will be
>>>>> some sort of bug which only triggers if these bits are set to 'false'?
>>>> I think I've suggested before that the cpuid hook should actually
>>>> return void, as it can't possibly fail (now that CPUID faulting is
>>>> being handled in generic code).
>>> This isn't about failing so much as it is about reporting the presence /
>>> absence of hardware features.  With this patch, cpuid unconditionally
>>> advertises the presence of a number of features (MOVBE, rtm, ADCX/ADOX,
>>> &c) because the emulation will work even if the features aren't actually
>>> present in hardware.  I'm suggesting that we may want to make sure that
>>> we test *both* the "feature is present" path, *and* the "feature is
>>> missing" path.
>> I have some plans to make this happen, but it isn't easy with the
>> existing infrastructure.  In the meantime, It is more important to get
>> better coverage.
> Can't we simply grab enough bits of input data to cover the ones
> of interest here, store them away, and use that instead of the
> hard coded 1s here?

Yes, but I also want to cover hiding features which are present on the
CPU as well, to test those early #UD paths.

~Andrew

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