[PATCH v2 for-4.9 0/7] x86/emul: Userspace fuzzing harness fixes

[PATCH v2 for-4.9 0/7] x86/emul: Userspace fuzzing harness fixes

[Andrew Cooper]

This is a subset of the previous fuzzing bugfix/improvement series, which is
the minimum required to avoid hitting assertions in the emulator.

From a 4.9 point of view, this entirely userspace testing harness changes (so
safe to take), but it allows us to sensibly fuzz the emulator in the
hypervisor (rather than hitting a load of assertions).

Andrew Cooper (7):
  MAINTAINERS: Move the x86 instruction emulator under x86 maintainership
  tools/insn-fuzz: Don't hit 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()

 MAINTAINERS                                     |   1 +
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 268 +++++++++++++++++-------
 tools/tests/x86_emulator/test_x86_emulator.c    |   8 +-
 3 files changed, 196 insertions(+), 81 deletions(-)

-- 
2.1.4


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

[PATCH v2 for-4.9 1/7] MAINTAINERS: Move the x86 instruction emulator under x86 maintainership

[Andrew Cooper]

Requested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
--
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c38bcd4..bee9d49 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -434,6 +434,7 @@ F:	xen/arch/x86/
 F:	xen/include/asm-x86/
 F:	tools/firmware/hvmloader/
 F:	tools/tests/x86_emulator/
+F:	tools/fuzz/x86_instruction_emulator/
 
 X86 I/O EMULATION
 M:	Paul Durrant <paul.durrant@citrix.com>
-- 
2.1.4


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

Re: [PATCH v2 for-4.9 1/7] MAINTAINERS: Move the x86 instruction emulator under x86 maintainership

[Wei Liu]

On Wed, Apr 05, 2017 at 06:53:27PM +0100, Andrew Cooper wrote:
> Requested-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v2 for-4.9 1/7] MAINTAINERS: Move the x86 instruction emulator under x86 maintainership

[Ian Jackson]

Andrew Cooper writes ("[PATCH v2 for-4.9 1/7] MAINTAINERS: Move the x86 instruction emulator under x86 maintainership"):
> Requested-by: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

[PATCH v2 for-4.9 2/7] tools/insn-fuzz: Don't hit 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 when passed to memcpy(), as passing NULL is undefined
behaviour per the C spec (irrespective of passing a size of 0).

Special case these fetches in fuzz_insn_fetch() before reaching data_read().

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

v2:
 * Rework in terms of special casing zero-length fetches only.
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 65c5a3b..64b7fb2 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -117,6 +117,16 @@ static int fuzz_insn_fetch(
     unsigned int bytes,
     struct x86_emulate_ctxt *ctxt)
 {
+    /*
+     * Zero-length instruction fetches are made at the destination of jumps,
+     * to perform segmentation checks.  No data needs returning.
+     */
+    if ( bytes == 0 )
+    {
+        assert(p_data == NULL);
+        return maybe_fail("insn_fetch", true);
+    }
+
     return data_read("insn_fetch", p_data, bytes);
 }
 
-- 
2.1.4


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

Re: [PATCH v2 for-4.9 2/7] tools/insn-fuzz: Don't hit memcpy() for zero-length reads

[Jan Beulich]

>>> On 05.04.17 at 19:53, <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 when passed to memcpy(), as passing NULL is undefined
> behaviour per the C spec (irrespective of passing a size of 0).
> 
> Special case these fetches in fuzz_insn_fetch() before reaching data_read().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: George Dunlap <george.dunlap@citrix.com>

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



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

[PATCH v2 for-4.9 3/7] 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>
Reviewed-by: 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 | 181 +++++++++++++++---------
 1 file changed, 116 insertions(+), 65 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 64b7fb2..db0719e 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 )
     {
-        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(
@@ -124,18 +146,19 @@ static int fuzz_insn_fetch(
     if ( bytes == 0 )
     {
         assert(p_data == NULL);
-        return maybe_fail("insn_fetch", true);
+        return maybe_fail(ctxt, "insn_fetch", true);
     }
 
-    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;
@@ -156,9 +179,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 )
     {
@@ -184,7 +208,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(
@@ -196,7 +220,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(
@@ -207,7 +231,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(
@@ -218,7 +242,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(
@@ -228,7 +252,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(
@@ -239,7 +263,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(
@@ -247,13 +271,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(
@@ -262,7 +286,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(
@@ -270,10 +294,13 @@ static int fuzz_read_segment(
     struct segment_register *reg,
     struct x86_emulate_ctxt *ctxt)
 {
+    const 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;
 }
@@ -283,15 +310,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;
 }
@@ -301,10 +330,13 @@ static int fuzz_read_cr(
     unsigned long *val,
     struct x86_emulate_ctxt *ctxt)
 {
-    if ( reg >= ARRAY_SIZE(input.cr) )
+    const 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;
 }
@@ -314,16 +346,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;
 }
@@ -355,6 +389,8 @@ static int fuzz_read_msr(
     uint64_t *val,
     struct x86_emulate_ctxt *ctxt)
 {
+    const struct fuzz_state *s = ctxt->data;
+    const struct fuzz_corpus *c = s->corpus;
     unsigned int idx;
 
     switch ( reg )
@@ -366,12 +402,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;
@@ -383,7 +419,7 @@ static int fuzz_read_msr(
     {
         if ( msr_index[idx] == reg )
         {
-            *val = input.msr[idx];
+            *val = c->msr[idx];
             return X86EMUL_OKAY;
         }
     }
@@ -396,10 +432,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;
 
@@ -414,7 +452,7 @@ static int fuzz_write_msr(
     {
         if ( msr_index[idx] == reg )
         {
-            input.msr[idx] = val;
+            c->msr[idx] = val;
             return X86EMUL_OKAY;
         }
     }
@@ -462,14 +500,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);
 
@@ -489,19 +529,25 @@ 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;
+    const 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;
+
     ctxt->lma = long_mode_active(ctxt);
 
     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;
     }
 }
 
@@ -561,9 +607,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);
@@ -612,11 +660,13 @@ static void disable_hooks(void)
  */
 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;
@@ -630,8 +680,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) )
@@ -640,8 +690,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;
     }
 }
 
@@ -659,7 +709,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 *),
@@ -668,8 +720,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 )
     {
@@ -685,11 +735,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);
 
     do {
         /* FIXME: Until we actually implement SIGFPE handling properly */
-- 
2.1.4


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

[PATCH v2 for-4.9 4/7] 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>
Reviewed-by: 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 db0719e..a20212e 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;
 };
 
 /*
@@ -461,7 +464,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),
@@ -603,7 +606,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");               \
     }
 
@@ -709,7 +712,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,
@@ -749,7 +754,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 v2 for-4.9 5/7] 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 | 45 ++++++++++++++++++++-----
 tools/tests/x86_emulator/test_x86_emulator.c    |  8 ++---
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index a20212e..fedeb9f 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -117,12 +117,15 @@ 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)
 {
+    /* Reads expected for all user and system segments. */
+    assert((unsigned int)seg < x86_seg_none);
+
     return data_read(ctxt, "read", p_data, bytes);
 }
 
@@ -136,12 +139,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);
+
     /*
      * Zero-length instruction fetches are made at the destination of jumps,
      * to perform segmentation checks.  No data needs returning.
@@ -211,6 +216,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);
 }
 
@@ -223,6 +230,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);
 }
 
@@ -234,6 +244,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);
 }
 
@@ -245,27 +257,43 @@ 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)
 {
+    /* Writes not expected for any system segments. */
+    assert(is_x86_user_segment(seg));
+
     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)
 {
+    /*
+     * Cmpxchg expected for user segments, and setting accessed/busy bits in
+     * GDT/LDT enties, but not expected for any IDT or TR accesses.
+     */
+    assert(is_x86_user_segment(seg) ||
+           seg == x86_seg_gdtr || seg == x86_seg_ldtr);
+
     return maybe_fail(ctxt, "cmpxchg", true);
 }
 
@@ -274,6 +302,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);
 }
 
@@ -300,8 +331,7 @@ static int fuzz_read_segment(
     const 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];
 
@@ -317,8 +347,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);
 
diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index efeb175..1992c3f 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -155,7 +155,7 @@ static const struct {
 static unsigned int bytes_read;
 
 static int read(
-    unsigned int seg,
+    enum x86_segment seg,
     unsigned long offset,
     void *p_data,
     unsigned int bytes,
@@ -210,7 +210,7 @@ static int read(
 }
 
 static int fetch(
-    unsigned int seg,
+    enum x86_segment seg,
     unsigned long offset,
     void *p_data,
     unsigned int bytes,
@@ -224,7 +224,7 @@ static int fetch(
 }
 
 static int write(
-    unsigned int seg,
+    enum x86_segment seg,
     unsigned long offset,
     void *p_data,
     unsigned int bytes,
@@ -240,7 +240,7 @@ static int write(
 }
 
 static int cmpxchg(
-    unsigned int seg,
+    enum x86_segment seg,
     unsigned long offset,
     void *old,
     void *new,
-- 
2.1.4


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

Re: [PATCH v2 for-4.9 5/7] tools/insn-fuzz: Correct hook prototypes, and assert() appropriate segments

[Jan Beulich]

>>> On 05.04.17 at 19:53, <andrew.cooper3@citrix.com> wrote:
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -117,12 +117,15 @@ 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)
>  {
> +    /* Reads expected for all user and system segments. */
> +    assert((unsigned int)seg < x86_seg_none);

Would this perhaps be more clear as
"is_user_segment() || is_system_segment()"?

> @@ -274,6 +302,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);

But no system segment, so rather
"is_user_segment() || seg == none"?

> @@ -300,8 +331,7 @@ static int fuzz_read_segment(
>      const 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);

Same as for fuzz_read().

> @@ -317,8 +347,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);

And here.

With at least the invlpg part taken care of
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

[PATCH v2 for-4.9 6/7] 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>
Reviewed-by: 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 | 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 fedeb9f..9e3a10a 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -402,7 +402,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] = {
@@ -413,7 +414,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 v2 for-4.9 7/7] 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>
Reviewed-by: 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 9e3a10a..8cf683d 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);
 
@@ -126,7 +141,7 @@ static int fuzz_read(
     /* Reads expected for all user and system segments. */
     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(
@@ -135,7 +150,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(
@@ -157,7 +172,7 @@ static int fuzz_insn_fetch(
         return maybe_fail(ctxt, "insn_fetch", true);
     }
 
-    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,
@@ -166,7 +181,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;
@@ -436,7 +451,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;
@@ -458,6 +473,7 @@ static int fuzz_read_msr(
         }
     }
 
+    x86_emul_hw_exception(13, 0, ctxt);
     return X86EMUL_EXCEPTION;
 }
 
@@ -491,6 +507,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