[PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[George Dunlap]

From: Jan Beulich <jbeulich@suse.com>

fuzz_insn_fetch() is the only data access helper where it is possible
to see offsets larger than 4Gb in 16- or 32-bit modes, as we leave the
incoming rIP untouched in the emulator itself. The check is needed here
as otherwise, after successfully fetching insn bytes, we may end up
zero-extending EIP soon after complete_insn, which collides with the
X86EMUL_EXCEPTION-conditional respective ASSERT() in
x86_emulate_wrapper(). (NB: put_rep_prefix() is what allows
complete_insn to be reached with rc set to other than X86EMUL_OKAY or
X86EMUL_DONE. See also commit 53f87c03b4 ["x86emul: generalize
exception handling for rep_* hooks"].)

Add assert()-s for all other (data) access routines, as effective
address generation in the emulator ought to guarantee in-range values.
For them to not trigger, several adjustments to the emulator's address
calculations are needed: While for DstBitBase it is really mandatory,
the specification allows for either behavior for two-part accesses.
Observed behavior on real hardware, however, is for such accesses to
silently wrap at the 2^^32 boundary in other than 64-bit mode, just
like they do at the 2^^64 boundary in 64-bit mode. While adding
truncate_ea() invocations there, also convert open coded instances of
it.

Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 32 ++++++++++++++++++++++---
 xen/arch/x86/x86_emulate/x86_emulate.c          | 22 +++++++++--------
 2 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index a2329f84a5..105145e9f9 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -139,7 +139,18 @@ static int fuzz_read(
     struct x86_emulate_ctxt *ctxt)
 {
     /* Reads expected for all user and system segments. */
-    assert(is_x86_user_segment(seg) || is_x86_system_segment(seg));
+    if ( is_x86_user_segment(seg) )
+        assert(ctxt->addr_size == 64 || !(offset >> 32));
+    else if ( seg == x86_seg_tr )
+        /*
+         * The TSS is special in that accesses below the segment base are
+         * possible, as the Interrupt Redirection Bitmap starts 32 bytes
+         * ahead of the I/O Bitmap, regardless of the value of the latter.
+         */
+        assert((long)offset < 0 ? (long)offset > -32 : !(offset >> 17));
+    else
+        assert(is_x86_system_segment(seg) &&
+               (ctxt->lma ? offset <= 0x10007 : !(offset >> 16)));
 
     return data_read(ctxt, seg, "read", p_data, bytes);
 }
@@ -162,6 +173,13 @@ static int fuzz_insn_fetch(
 {
     assert(seg == x86_seg_cs);
 
+    /* Minimal segment limit checking, until full one is being put in place. */
+    if ( ctxt->addr_size < 64 && (offset >> 32) )
+    {
+        x86_emul_hw_exception(13, 0, ctxt);
+        return X86EMUL_EXCEPTION;
+    }
+
     /*
      * Zero-length instruction fetches are made at the destination of jumps,
      * to perform segmentation checks.  No data needs returning.
@@ -232,6 +250,7 @@ static int fuzz_rep_ins(
     struct x86_emulate_ctxt *ctxt)
 {
     assert(dst_seg == x86_seg_es);
+    assert(ctxt->addr_size == 64 || !(dst_offset >> 32));
 
     return _fuzz_rep_read(ctxt, "rep_ins", reps);
 }
@@ -247,6 +266,7 @@ static int fuzz_rep_movs(
 {
     assert(is_x86_user_segment(src_seg));
     assert(dst_seg == x86_seg_es);
+    assert(ctxt->addr_size == 64 || !((src_offset | dst_offset) >> 32));
 
     return _fuzz_rep_read(ctxt, "rep_movs", reps);
 }
@@ -260,6 +280,7 @@ static int fuzz_rep_outs(
     struct x86_emulate_ctxt *ctxt)
 {
     assert(is_x86_user_segment(src_seg));
+    assert(ctxt->addr_size == 64 || !(src_offset >> 32));
 
     return _fuzz_rep_write(ctxt, "rep_outs", reps);
 }
@@ -277,6 +298,7 @@ static int fuzz_rep_stos(
      * for CLZERO.
      */
     assert(is_x86_user_segment(seg));
+    assert(ctxt->addr_size == 64 || !(offset >> 32));
 
     return _fuzz_rep_write(ctxt, "rep_stos", reps);
 }
@@ -290,6 +312,7 @@ static int fuzz_write(
 {
     /* Writes not expected for any system segments. */
     assert(is_x86_user_segment(seg));
+    assert(ctxt->addr_size == 64 || !(offset >> 32));
 
     return maybe_fail(ctxt, "write", true);
 }
@@ -306,8 +329,10 @@ static int fuzz_cmpxchg(
      * 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);
+    if ( is_x86_user_segment(seg) )
+        assert(ctxt->addr_size == 64 || !(offset >> 32));
+    else
+        assert((seg == x86_seg_gdtr || seg == x86_seg_ldtr) && !(offset >> 16));
 
     return maybe_fail(ctxt, "cmpxchg", true);
 }
@@ -319,6 +344,7 @@ static int fuzz_invlpg(
 {
     /* invlpg(), unlike all other hooks, may be called with x86_seg_none. */
     assert(is_x86_user_segment(seg) || seg == x86_seg_none);
+    assert(ctxt->addr_size == 64 || !(offset >> 32));
 
     return maybe_fail(ctxt, "invlpg", false);
 }
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index c1e2300b39..31df5aeb97 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1249,10 +1249,10 @@ static void __put_rep_prefix(
 
 /* Clip maximum repetitions so that the index register at most just wraps. */
 #define truncate_ea_and_reps(ea, reps, bytes_per_rep) ({                  \
-    unsigned long todo__, ea__ = truncate_word(ea, ad_bytes);             \
+    unsigned long todo__, ea__ = truncate_ea(ea);                         \
     if ( !(_regs.eflags & X86_EFLAGS_DF) )                                \
-        todo__ = truncate_word(-(ea), ad_bytes) / (bytes_per_rep);        \
-    else if ( truncate_word((ea) + (bytes_per_rep) - 1, ad_bytes) < ea__ )\
+        todo__ = truncate_ea(-ea__) / (bytes_per_rep);                    \
+    else if ( truncate_ea(ea__ + (bytes_per_rep) - 1) < ea__ )            \
         todo__ = 1;                                                       \
     else                                                                  \
         todo__ = ea__ / (bytes_per_rep) + 1;                              \
@@ -3136,6 +3136,7 @@ x86_emulate(
                     op_bytes + (((-src.val - 1) >> 3) & ~(op_bytes - 1L));
             else
                 ea.mem.off += (src.val >> 3) & ~(op_bytes - 1L);
+            ea.mem.off = truncate_ea(ea.mem.off);
         }
 
         /* Bit index always truncated to within range. */
@@ -3354,7 +3355,7 @@ x86_emulate(
         unsigned long src_val2;
         int lb, ub, idx;
         generate_exception_if(src.type != OP_MEM, EXC_UD);
-        if ( (rc = read_ulong(src.mem.seg, src.mem.off + op_bytes,
+        if ( (rc = read_ulong(src.mem.seg, truncate_ea(src.mem.off + op_bytes),
                               &src_val2, op_bytes, ctxt, ops)) )
             goto done;
         ub  = (op_bytes == 2) ? (int16_t)src_val2 : (int32_t)src_val2;
@@ -3905,7 +3906,7 @@ x86_emulate(
         seg = (b & 1) * 3; /* es = 0, ds = 3 */
     les:
         generate_exception_if(src.type != OP_MEM, EXC_UD);
-        if ( (rc = read_ulong(src.mem.seg, src.mem.off + src.bytes,
+        if ( (rc = read_ulong(src.mem.seg, truncate_ea(src.mem.off + src.bytes),
                               &dst.val, 2, ctxt, ops)) != X86EMUL_OKAY )
             goto done;
         ASSERT(is_x86_user_segment(seg));
@@ -4939,7 +4940,8 @@ x86_emulate(
         case 5: /* jmp (far, absolute indirect) */
             generate_exception_if(src.type != OP_MEM, EXC_UD);
 
-            if ( (rc = read_ulong(src.mem.seg, src.mem.off + op_bytes,
+            if ( (rc = read_ulong(src.mem.seg,
+                                  truncate_ea(src.mem.off + op_bytes),
                                   &imm2, 2, ctxt, ops)) )
                 goto done;
             imm1 = src.val;
@@ -5126,8 +5128,8 @@ x86_emulate(
             }
             if ( (rc = ops->write(ea.mem.seg, ea.mem.off, &sreg.limit,
                                   2, ctxt)) != X86EMUL_OKAY ||
-                 (rc = ops->write(ea.mem.seg, ea.mem.off + 2, &sreg.base,
-                                  op_bytes, ctxt)) != X86EMUL_OKAY )
+                 (rc = ops->write(ea.mem.seg, truncate_ea(ea.mem.off + 2),
+                                  &sreg.base, op_bytes, ctxt)) != X86EMUL_OKAY )
                 goto done;
             break;
 
@@ -5137,9 +5139,9 @@ x86_emulate(
             generate_exception_if(!mode_ring0(), EXC_GP, 0);
             fail_if(ops->write_segment == NULL);
             memset(&sreg, 0, sizeof(sreg));
-            if ( (rc = read_ulong(ea.mem.seg, ea.mem.off+0,
+            if ( (rc = read_ulong(ea.mem.seg, ea.mem.off,
                                   &limit, 2, ctxt, ops)) ||
-                 (rc = read_ulong(ea.mem.seg, ea.mem.off+2,
+                 (rc = read_ulong(ea.mem.seg, truncate_ea(ea.mem.off + 2),
                                   &base, mode_64bit() ? 8 : 4, ctxt, ops)) )
                 goto done;
             generate_exception_if(!is_canonical_address(base), EXC_GP, 0);
-- 
2.14.1


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

[PATCH v2 02/13] fuzz/x86_emulate: Actually use cpu_regs input

[George Dunlap]

Commit c07574b reorganized the way fuzzing was done, explicitly
creating a structure that the input data would be copied into.

Unfortunately, the cpu register state used by the emulator is on the
stack; it's cleared, but data is never copied into it.

If we're explicitly setting an entirely new cpu_regs struct for each
new input anyway, there's no need to have two copies around anymore;
just point to the one in the data structure.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---

This is a candidate for backporting to 4.9.

To test that this has an effect, revert the previous patch
("x86emul/fuzz: add rudimentary limit checking"): with this patch it
hits an ASSERT().

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 105145e9f9..48a879cc88 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -785,13 +785,12 @@ 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 = {
         .ops = all_fuzzer_ops,
     };
     struct x86_emulate_ctxt ctxt = {
         .data = &state,
-        .regs = &regs,
+        .regs = &input.regs,
         .addr_size = 8 * sizeof(void *),
         .sp_size = 8 * sizeof(void *),
     };
-- 
2.14.1


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

[PATCH v2 03/13] fuzz/x86_emulate: Clear errors after each iteration

[George Dunlap]

Once feof() returns true for a stream, it will continue to return true
for that stream until clearerr() is called (or the stream is closed
and re-opened).

In llvm-clang-fast-mode, the same file descriptor is used for each
iteration of the loop, meaning that the "Input too large" check was
broken -- feof() would return true even if the fread() hadn't hit the
end of the file.  The result is that AFL generates testcases of
arbitrary size.

Fix this by clearing the error after each iteration.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
Changes in v2:
- Actually fix the root issue rather than working around it

This is a candidate for backport to 4.9.

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/afl-harness.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/fuzz/x86_instruction_emulator/afl-harness.c b/tools/fuzz/x86_instruction_emulator/afl-harness.c
index 154869336a..b4d15451b5 100644
--- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
+++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
@@ -97,6 +97,8 @@ int main(int argc, char **argv)
             fclose(fp);
             fp = NULL;
         }
+        else
+            clearerr(fp);
 
         LLVMFuzzerTestOneInput(input, size);
     }
-- 
2.14.1


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

[PATCH v2 04/13] fuzz/x86_emulate: Improve failure descriptions in x86_emulate harness

[George Dunlap]

- Print the symbolic name rather than the number
- Explicitly state when data_read() fails due to EOI

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
Changes in v2:
- Add spaces around '='

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 48a879cc88..761b2ae96e 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -52,6 +52,14 @@ struct fuzz_state
     struct x86_emulate_ops ops;
 };
 
+char *x86emul_return_string[] = {
+    [X86EMUL_OKAY] = "X86EMUL_OKAY",
+    [X86EMUL_UNHANDLEABLE] = "X86EMUL_UNHANDLEABLE",
+    [X86EMUL_EXCEPTION] = "X86EMUL_EXCEPTION",
+    [X86EMUL_RETRY] = "X86EMUL_RETRY",
+    [X86EMUL_DONE] = "X86EMUL_DONE",
+};
+
 /*
  * Randomly return success or failure when processing data.  If
  * `exception` is false, this function turns _EXCEPTION to _OKAY.
@@ -84,7 +92,7 @@ static int maybe_fail(struct x86_emulate_ctxt *ctxt,
     if ( rc == X86EMUL_EXCEPTION && !exception )
         rc = X86EMUL_OKAY;
 
-    printf("maybe_fail %s: %d\n", why, rc);
+    printf("maybe_fail %s: %s\n", why, x86emul_return_string[rc]);
 
     if ( rc == X86EMUL_EXCEPTION )
         /* Fake up a pagefault. */
@@ -113,6 +121,7 @@ static int data_read(struct x86_emulate_ctxt *ctxt,
             x86_emul_hw_exception(13, 0, ctxt);
 
         rc = X86EMUL_EXCEPTION;
+        printf("data_read %s: X86EMUL_EXCEPTION (end of input)\n", why);
     }
     else
         rc = maybe_fail(ctxt, why, true);
-- 
2.14.1


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

[PATCH v2 05/13] fuzz/x86_emulate: Implement input_read() and input_avail()

[George Dunlap]

Rather than open-coding the "read" from the input file.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2:
- Use less dread-ful names
- Return bool rather than int

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 31 ++++++++++++++++++-------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 761b2ae96e..92684cf088 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -52,6 +52,22 @@ struct fuzz_state
     struct x86_emulate_ops ops;
 };
 
+static inline bool input_available(struct fuzz_state *s, size_t size)
+{
+    return s->data_index + size < s->data_num;
+}
+
+static inline bool input_read(struct fuzz_state *s, void *dst, size_t size)
+{
+    if ( !input_available(s, size) )
+        return 0;
+
+    memcpy(dst, &s->corpus->data[s->data_index], size);
+    s->data_index += size;
+
+    return 1;
+}
+
 char *x86emul_return_string[] = {
     [X86EMUL_OKAY] = "X86EMUL_OKAY",
     [X86EMUL_UNHANDLEABLE] = "X86EMUL_UNHANDLEABLE",
@@ -68,10 +84,10 @@ 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;
+    unsigned char c;
     int rc;
 
-    if ( s->data_index >= s->data_num )
+    if ( !input_read(s, &c, sizeof(c)) )
         rc = X86EMUL_EXCEPTION;
     else
     {
@@ -80,13 +96,12 @@ static int maybe_fail(struct x86_emulate_ctxt *ctxt,
          * 25% unhandlable
          * 25% exception
          */
-        if ( c->data[s->data_index] > 0xc0 )
+        if ( c > 0xc0 )
             rc = X86EMUL_EXCEPTION;
-        else if ( c->data[s->data_index] > 0x80 )
+        else if ( c > 0x80 )
             rc = X86EMUL_UNHANDLEABLE;
         else
             rc = X86EMUL_OKAY;
-        s->data_index++;
     }
 
     if ( rc == X86EMUL_EXCEPTION && !exception )
@@ -106,11 +121,10 @@ 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 ( s->data_index + bytes > s->data_num )
+    if ( !input_available(s, bytes) )
     {
         /*
          * Fake up a segment limit violation.  System segment limit volations
@@ -128,8 +142,7 @@ static int data_read(struct x86_emulate_ctxt *ctxt,
 
     if ( rc == X86EMUL_OKAY )
     {
-        memcpy(dst, &c->data[s->data_index], bytes);
-        s->data_index += bytes;
+        input_read(s, dst, bytes);
 
         printf("%s: ", why);
         for ( i = 0; i < bytes; i++ )
-- 
2.14.1


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

[PATCH v2 06/13] fuzz/x86_emulate: Rename the file containing the wrapper code

[George Dunlap]

When generating coverage output, by default gcov generates output
filenames based only on the coverage file and the "leaf" source file,
not the full path.  As a result, it uses the same name for
x86_emulate.c and x86_emulate/x86_emulate.c, generally overwriting the
second (which we actually are about) with the first (which is just a
wrapper).

Rename the user-space wrapper helpers to x86_emulate_user.[ch], so
that it generates separate files.

There is actually an option to gcov, `--preserve-paths`, which will
cause the full path name to be included in the filename, properly
distinguishing between the two.  However, given that the user-space
wrapper doesn't actually do any emulation (and the poor state of gcov
documentation making it difficult to find the option in the first
place), it seems to make more sense to rename the file anyway.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---

NB: I discovered the `-p` option to gcov after writing this patch.
But I think the patch itself still makes sense.

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/Makefile                 | 12 ++++++------
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c              |  2 +-
 tools/tests/x86_emulator/Makefile                            |  6 +++---
 tools/tests/x86_emulator/test_x86_emulator.c                 |  2 +-
 .../tests/x86_emulator/{x86_emulate.c => x86_emulate_user.c} |  2 +-
 .../tests/x86_emulator/{x86_emulate.h => x86_emulate_user.h} |  0
 6 files changed, 12 insertions(+), 12 deletions(-)
 rename tools/tests/x86_emulator/{x86_emulate.c => x86_emulate_user.c} (99%)
 rename tools/tests/x86_emulator/{x86_emulate.h => x86_emulate_user.h} (100%)

diff --git a/tools/fuzz/x86_instruction_emulator/Makefile b/tools/fuzz/x86_instruction_emulator/Makefile
index a3f6b2c754..10009dc08f 100644
--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -18,22 +18,22 @@ asm:
 
 asm/%: asm ;
 
-x86_emulate.c x86_emulate.h: %:
+x86_emulate_user.c x86_emulate_user.h: %:
 	[ -L $* ] || ln -sf $(XEN_ROOT)/tools/tests/x86_emulator/$*
 
 CFLAGS += $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -I.
 
 x86.h := asm/x86-vendors.h asm/x86-defns.h asm/msr-index.h
-x86_emulate.h := x86_emulate.h x86_emulate/x86_emulate.h $(x86.h)
+x86_emulate.h := x86_emulate_user.h x86_emulate/x86_emulate.h $(x86.h)
 
-x86_emulate.o: x86_emulate.c x86_emulate/x86_emulate.c $(x86_emulate.h)
+x86_emulate_user.o: x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
 
 fuzz-emul.o: $(x86_emulate.h)
 
-x86-insn-fuzzer.a: fuzz-emul.o x86_emulate.o
+x86-insn-fuzzer.a: fuzz-emul.o x86_emulate_user.o
 	$(AR) rc $@ $^
 
-afl-harness: afl-harness.o fuzz-emul.o x86_emulate.o
+afl-harness: afl-harness.o fuzz-emul.o x86_emulate_user.o
 	$(CC) $(CFLAGS) $^ -o $@
 
 # Common targets
@@ -42,7 +42,7 @@ all: x86-insn-fuzz-all
 
 .PHONY: distclean
 distclean: clean
-	rm -f x86_emulate x86_emulate.c x86_emulate.h asm
+	rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm
 
 .PHONY: clean
 clean:
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 92684cf088..dc180b070c 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -15,7 +15,7 @@
 #include <unistd.h>
 #include <xen/xen.h>
 
-#include "x86_emulate.h"
+#include "x86_emulate_user.h"
 
 #define MSR_INDEX_MAX 16
 
diff --git a/tools/tests/x86_emulator/Makefile b/tools/tests/x86_emulator/Makefile
index fd13ab53b1..888495a6a2 100644
--- a/tools/tests/x86_emulator/Makefile
+++ b/tools/tests/x86_emulator/Makefile
@@ -75,7 +75,7 @@ $(addsuffix .h,$(TESTCASES)): %.h: %.c testcase.mk Makefile
 $(addsuffix .c,$(SIMD)) $(addsuffix -avx.c,$(filter sse%,$(SIMD))):
 	ln -sf simd.c $@
 
-$(TARGET): x86_emulate.o test_x86_emulator.o
+$(TARGET): x86_emulate_user.o test_x86_emulator.o
 	$(HOSTCC) -o $@ $^
 
 .PHONY: clean
@@ -101,9 +101,9 @@ asm/%: asm ;
 HOSTCFLAGS += $(CFLAGS_xeninclude) -I.
 
 x86.h := asm/x86-vendors.h asm/x86-defns.h asm/msr-index.h
-x86_emulate.h := x86_emulate.h x86_emulate/x86_emulate.h $(x86.h)
+x86_emulate.h := x86_emulate_user.h x86_emulate/x86_emulate.h $(x86.h)
 
-x86_emulate.o: x86_emulate.c x86_emulate/x86_emulate.c $(x86_emulate.h)
+x86_emulate_user.o: x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
 	$(HOSTCC) $(HOSTCFLAGS) -D__XEN_TOOLS__ -c -g -o $@ $<
 
 test_x86_emulator.o: test_x86_emulator.c $(addsuffix .h,$(TESTCASES)) $(x86_emulate.h)
diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index 4371e467e6..4d9c781610 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -3,7 +3,7 @@
 #include <stdio.h>
 #include <sys/mman.h>
 
-#include "x86_emulate.h"
+#include "x86_emulate_user.h"
 #include "blowfish.h"
 #include "sse.h"
 #include "sse2.h"
diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate_user.c
similarity index 99%
rename from tools/tests/x86_emulator/x86_emulate.c
rename to tools/tests/x86_emulator/x86_emulate_user.c
index 79661d5c2b..adae6950c8 100644
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate_user.c
@@ -1,4 +1,4 @@
-#include "x86_emulate.h"
+#include "x86_emulate_user.h"
 
 #include <sys/mman.h>
 
diff --git a/tools/tests/x86_emulator/x86_emulate.h b/tools/tests/x86_emulator/x86_emulate_user.h
similarity index 100%
rename from tools/tests/x86_emulator/x86_emulate.h
rename to tools/tests/x86_emulator/x86_emulate_user.h
-- 
2.14.1


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

[PATCH v2 07/13] fuzz/x86_emulate: Add 'afl-cov' target

[George Dunlap]

...to generate a "normal" coverage-instrumented binary, suitable for
use with gcov or afl-cov.

This is slightly annoying because:

 - Every object file needs to have been instrumented to work
   effectively

 - You generally want to have both an afl-instrumented binary and a
   gcov-instrumented binary at the same time, but

 - gcov instrumentation and afl instrumentation are mutually exclusive

So when making the `afl-cov` target, generate a second set of object
files and a second binary with the `-cov` suffix.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
Changes in v2:
- Pull 'inputs' to x86_emulate_user* into a make variable to avoid duplication

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 .gitignore                                   |  1 +
 tools/fuzz/README.afl                        | 14 ++++++++++++++
 tools/fuzz/x86_instruction_emulator/Makefile | 22 ++++++++++++++++++++--
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/.gitignore b/.gitignore
index cc16649457..5c8ed26b87 100644
--- a/.gitignore
+++ b/.gitignore
@@ -162,6 +162,7 @@ tools/fuzz/libelf/afl-libelf-fuzzer
 tools/fuzz/x86_instruction_emulator/asm
 tools/fuzz/x86_instruction_emulator/x86_emulate*
 tools/fuzz/x86_instruction_emulator/afl-harness
+tools/fuzz/x86_instruction_emulator/afl-harness-cov
 tools/helpers/_paths.h
 tools/helpers/init-xenstore-domain
 tools/helpers/xen-init-dom0
diff --git a/tools/fuzz/README.afl b/tools/fuzz/README.afl
index 4758de2490..0d955b2687 100644
--- a/tools/fuzz/README.afl
+++ b/tools/fuzz/README.afl
@@ -41,3 +41,17 @@ Use the x86 instruction emulator fuzzer as an example.
    $ $AFLPATH/afl-fuzz -t 1000 -i testcase_dir -o findings_dir -- ./afl-harness
 
 Please see AFL documentation for more information.
+
+# GENERATING COVERAGE INFORMATION
+
+To use afl-cov or gcov, you need a separate binary instrumented to
+generate coverage data.  To do this, use the target `afl-cov`:
+
+    $ make afl-cov #produces afl-harness-cov
+
+NOTE: Please also note that the coverage instrumentation hard-codes
+the absolute path for the instrumentation read and write files in the
+binary; so coverage data will always show up in the build directory no
+matter where you run the binary from.
+
+Please see afl-cov and/or gcov documentation for more information.
\ No newline at end of file
diff --git a/tools/fuzz/x86_instruction_emulator/Makefile b/tools/fuzz/x86_instruction_emulator/Makefile
index 10009dc08f..4e829dfcbc 100644
--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -23,19 +23,34 @@ x86_emulate_user.c x86_emulate_user.h: %:
 
 CFLAGS += $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -I.
 
+GCOV_FLAGS=--coverage
+
 x86.h := asm/x86-vendors.h asm/x86-defns.h asm/msr-index.h
 x86_emulate.h := x86_emulate_user.h x86_emulate/x86_emulate.h $(x86.h)
 
-x86_emulate_user.o: x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
+X86_EMULATE_INPUTS = x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
+x86_emulate_user.o: $(X86_EMULATE_INPUTS)
+
+x86_emulate_user-cov.o: $(X86_EMULATE_INPUTS)
+	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) -o $@ x86_emulate_user.c
 
 fuzz-emul.o: $(x86_emulate.h)
 
+fuzz-emul-cov.o: fuzz-emul.c $(x86_emulate.h)
+	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) -o $@ fuzz-emul.c
+
+afl-harness-cov.o: afl-harness.c
+	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) $^ -o $@
+
 x86-insn-fuzzer.a: fuzz-emul.o x86_emulate_user.o
 	$(AR) rc $@ $^
 
 afl-harness: afl-harness.o fuzz-emul.o x86_emulate_user.o
 	$(CC) $(CFLAGS) $^ -o $@
 
+afl-harness-cov: afl-harness-cov.o fuzz-emul-cov.o x86_emulate_user-cov.o
+	$(CC) $(CFLAGS) $(GCOV_FLAGS) $^ -o $@
+
 # Common targets
 .PHONY: all
 all: x86-insn-fuzz-all
@@ -46,7 +61,7 @@ distclean: clean
 
 .PHONY: clean
 clean:
-	rm -f *.a *.o .*.d afl-harness
+	rm -f *.a *.o .*.d afl-harness afl-harness-cov *.gcda *.gcno *.gcov
 
 .PHONY: install
 install: all
@@ -55,3 +70,6 @@ install: all
 
 .PHONY: afl
 afl: afl-harness
+
+.PHONY: afl-cov
+afl-cov: afl-harness-cov
-- 
2.14.1


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

[PATCH v2 08/13] fuzz/x86_emulate: Take multiple test files for inputs

[George Dunlap]

Finding aggregate coverage for a set of test files means running each
afl-generated test case through the harness.  At the moment, this is
done by re-executing afl-harness-cov with each input file.  When a
large number of test cases have been generated, this can take a
significant amonut of time; a recent test with 30k total files
generated by 4 parallel fuzzers took over 7 minutes.

The vast majority of this time is taken up with 'exec', however.
Since the harness is already designed to loop over multiple inputs for
llvm "persistent mode", just allow it to take a large number of inputs
on the same when *not* running in llvm "persistent mode"..  Then the
command can be efficiently executed like this:

  ls */queue/id* | xargs $path/afl-harness-cov

For the above-mentioned test on 30k files, the time to generate
coverage data was reduced from 7 minutes to under 30 seconds.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2:
- Make check for batch processing more clear

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/README.afl                             |  7 +++++++
 tools/fuzz/x86_instruction_emulator/afl-harness.c | 24 ++++++++++++++++-------
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/tools/fuzz/README.afl b/tools/fuzz/README.afl
index 0d955b2687..e8c23d734c 100644
--- a/tools/fuzz/README.afl
+++ b/tools/fuzz/README.afl
@@ -49,6 +49,13 @@ generate coverage data.  To do this, use the target `afl-cov`:
 
     $ make afl-cov #produces afl-harness-cov
 
+In order to speed up the process of checking total coverage,
+`afl-harness-cov` can take several test inputs on its command-line;
+the speed-up effect should be similar to that of using afl-clang-fast.
+You can use xargs to do this most efficiently, like so:
+
+    $ ls queue/id* | xargs $path/afl-harness-cov
+
 NOTE: Please also note that the coverage instrumentation hard-codes
 the absolute path for the instrumentation read and write files in the
 binary; so coverage data will always show up in the build directory no
diff --git a/tools/fuzz/x86_instruction_emulator/afl-harness.c b/tools/fuzz/x86_instruction_emulator/afl-harness.c
index b4d15451b5..669f698711 100644
--- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
+++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
@@ -16,6 +16,8 @@ int main(int argc, char **argv)
 {
     size_t size;
     FILE *fp = NULL;
+    int count = 0;
+    int max;
 
     setbuf(stdin, NULL);
     setbuf(stdout, NULL);
@@ -42,8 +44,7 @@ int main(int argc, char **argv)
             break;
 
         case '?':
-        usage:
-            printf("Usage: %s $FILE | [--min-input-size]\n", argv[0]);
+            printf("Usage: %s $FILE [$FILE...] | [--min-input-size]\n", argv[0]);
             exit(-1);
             break;
 
@@ -54,10 +55,13 @@ int main(int argc, char **argv)
         }
     }
 
-    if ( optind == argc ) /* No positional parameters.  Use stdin. */
+    max = argc - optind;
+
+    if ( !max ) /* No positional parameters.  Use stdin. */
+    {
+        max = 1;
         fp = stdin;
-    else if ( optind != (argc - 1) )
-        goto usage;
+    }
 
     if ( LLVMFuzzerInitialize(&argc, &argv) )
         exit(-1);
@@ -66,11 +70,14 @@ int main(int argc, char **argv)
     __AFL_INIT();
 
     while ( __AFL_LOOP(1000) )
+#else
+    for( count = 0; count < max; count++ )
 #endif
     {
         if ( fp != stdin ) /* If not using stdin, open the provided file. */
         {
-            fp = fopen(argv[optind], "rb");
+            printf("Opening file %s\n", argv[optind]);
+            fp = fopen(argv[optind + count], "rb");
             if ( fp == NULL )
             {
                 perror("fopen");
@@ -89,7 +96,10 @@ int main(int argc, char **argv)
         if ( !feof(fp) )
         {
             printf("Input too large\n");
-            exit(-1);
+            /* Don't exit if we're doing batch processing */
+            if ( max == 1 )
+                exit(-1);
+            continue;
         }
 
         if ( fp != stdin )
-- 
2.14.1


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

[PATCH v2 09/13] fuzz/x86_emulate: Move all state into fuzz_state

[George Dunlap]

This is in preparation for adding the option for a more "compact"
interpretation of the fuzzing data, in which we only change select
bits of the state.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2: Port over previous changes

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 90 +++++++++++++------------
 1 file changed, 46 insertions(+), 44 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index dc180b070c..c8a5507f8b 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -24,14 +24,8 @@
 /* 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;
-    struct segment_register segments[SEG_NUM];
-    unsigned long options;
     unsigned char data[4096];
 } input;
-#define DATA_OFFSET offsetof(struct fuzz_corpus, data)
 
 /*
  * Internal state of the fuzzing harness.  Calculated initially from the input
@@ -39,6 +33,12 @@ struct fuzz_corpus
  */
 struct fuzz_state
 {
+    unsigned long options;
+    unsigned long cr[5];
+    uint64_t msr[MSR_INDEX_MAX];
+    struct segment_register segments[SEG_NUM];
+    struct cpu_user_regs regs;
+
     /* Fuzzer's input data. */
     struct fuzz_corpus *corpus;
 
@@ -51,6 +51,8 @@ struct fuzz_state
     /* Emulation ops, some of which are disabled based on corpus->options. */
     struct x86_emulate_ops ops;
 };
+#define DATA_OFFSET offsetof(struct fuzz_state, corpus)
+
 
 static inline bool input_available(struct fuzz_state *s, size_t size)
 {
@@ -392,11 +394,10 @@ static int fuzz_read_segment(
     struct x86_emulate_ctxt *ctxt)
 {
     const struct fuzz_state *s = ctxt->data;
-    const struct fuzz_corpus *c = s->corpus;
 
     assert(is_x86_user_segment(seg) || is_x86_system_segment(seg));
 
-    *reg = c->segments[seg];
+    *reg = s->segments[seg];
 
     return X86EMUL_OKAY;
 }
@@ -407,7 +408,6 @@ static int fuzz_write_segment(
     struct x86_emulate_ctxt *ctxt)
 {
     struct fuzz_state *s = ctxt->data;
-    struct fuzz_corpus *c = s->corpus;
     int rc;
 
     assert(is_x86_user_segment(seg) || is_x86_system_segment(seg));
@@ -415,7 +415,7 @@ static int fuzz_write_segment(
     rc = maybe_fail(ctxt, "write_segment", true);
 
     if ( rc == X86EMUL_OKAY )
-        c->segments[seg] = *reg;
+        s->segments[seg] = *reg;
 
     return rc;
 }
@@ -426,12 +426,11 @@ static int fuzz_read_cr(
     struct x86_emulate_ctxt *ctxt)
 {
     const struct fuzz_state *s = ctxt->data;
-    const struct fuzz_corpus *c = s->corpus;
 
-    if ( reg >= ARRAY_SIZE(c->cr) )
+    if ( reg >= ARRAY_SIZE(s->cr) )
         return X86EMUL_UNHANDLEABLE;
 
-    *val = c->cr[reg];
+    *val = s->cr[reg];
 
     return X86EMUL_OKAY;
 }
@@ -442,17 +441,16 @@ static int fuzz_write_cr(
     struct x86_emulate_ctxt *ctxt)
 {
     struct fuzz_state *s = ctxt->data;
-    struct fuzz_corpus *c = s->corpus;
     int rc;
 
-    if ( reg >= ARRAY_SIZE(c->cr) )
+    if ( reg >= ARRAY_SIZE(s->cr) )
         return X86EMUL_UNHANDLEABLE;
 
     rc = maybe_fail(ctxt, "write_cr", true);
     if ( rc != X86EMUL_OKAY )
         return rc;
 
-    c->cr[reg] = val;
+    s->cr[reg] = val;
 
     return X86EMUL_OKAY;
 }
@@ -487,7 +485,6 @@ static int fuzz_read_msr(
     struct x86_emulate_ctxt *ctxt)
 {
     const struct fuzz_state *s = ctxt->data;
-    const struct fuzz_corpus *c = s->corpus;
     unsigned int idx;
 
     switch ( reg )
@@ -501,10 +498,10 @@ static int fuzz_read_msr(
          */
         return data_read(ctxt, x86_seg_none, "read_msr", val, sizeof(*val));
     case MSR_EFER:
-        *val = c->msr[MSRI_EFER];
+        *val = s->msr[MSRI_EFER];
         *val &= ~EFER_LMA;
-        if ( (*val & EFER_LME) && (c->cr[4] & X86_CR4_PAE) &&
-             (c->cr[0] & X86_CR0_PG) )
+        if ( (*val & EFER_LME) && (s->cr[4] & X86_CR4_PAE) &&
+             (s->cr[0] & X86_CR0_PG) )
         {
             printf("Setting EFER_LMA\n");
             *val |= EFER_LMA;
@@ -516,7 +513,7 @@ static int fuzz_read_msr(
     {
         if ( msr_index[idx] == reg )
         {
-            *val = c->msr[idx];
+            *val = s->msr[idx];
             return X86EMUL_OKAY;
         }
     }
@@ -531,7 +528,6 @@ static int fuzz_write_msr(
     struct x86_emulate_ctxt *ctxt)
 {
     struct fuzz_state *s = ctxt->data;
-    struct fuzz_corpus *c = s->corpus;
     unsigned int idx;
     int rc;
 
@@ -550,7 +546,7 @@ static int fuzz_write_msr(
     {
         if ( msr_index[idx] == reg )
         {
-            c->msr[idx] = val;
+            s->msr[idx] = val;
             return X86EMUL_OKAY;
         }
     }
@@ -600,15 +596,14 @@ 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", c->cr[0]);
-    printf(" cr3: %lx\n", c->cr[3]);
-    printf(" cr4: %lx\n", c->cr[4]);
+    printf(" cr0: %lx\n", s->cr[0]);
+    printf(" cr3: %lx\n", s->cr[3]);
+    printf(" cr4: %lx\n", s->cr[4]);
 
     printf(" rip: %"PRIx64"\n", regs->rip);
 
@@ -629,15 +624,13 @@ static bool long_mode_active(struct x86_emulate_ctxt *ctxt)
 static bool in_longmode(struct x86_emulate_ctxt *ctxt)
 {
     const struct fuzz_state *s = ctxt->data;
-    const struct fuzz_corpus *c = s->corpus;
 
-    return long_mode_active(ctxt) && c->segments[x86_seg_cs].l;
+    return long_mode_active(ctxt) && s->segments[x86_seg_cs].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);
 
@@ -645,11 +638,20 @@ static void set_sizes(struct x86_emulate_ctxt *ctxt)
         ctxt->addr_size = ctxt->sp_size = 64;
     else
     {
-        ctxt->addr_size = c->segments[x86_seg_cs].db ? 32 : 16;
-        ctxt->sp_size   = c->segments[x86_seg_ss].db ? 32 : 16;
+        ctxt->addr_size = s->segments[x86_seg_cs].db ? 32 : 16;
+        ctxt->sp_size   = s->segments[x86_seg_ss].db ? 32 : 16;
     }
 }
 
+static void setup_state(struct x86_emulate_ctxt *ctxt)
+{
+    struct fuzz_state *s = ctxt->data;
+
+    /* Fuzz all of the state in one go */
+    if (!input_read(s, s, DATA_OFFSET))
+        exit(-1);
+}
+
 #define CANONICALIZE(x)                                   \
     do {                                                  \
         uint64_t _y = (x);                                \
@@ -709,8 +711,7 @@ enum {
 static void disable_hooks(struct x86_emulate_ctxt *ctxt)
 {
     struct fuzz_state *s = ctxt->data;
-    const struct fuzz_corpus *c = s->corpus;
-    unsigned long bitmap = c->options;
+    unsigned long bitmap = s->options;
 
     /* See also sanitize_input, some hooks can't be disabled. */
     MAYBE_DISABLE_HOOK(read);
@@ -760,12 +761,11 @@ static void disable_hooks(struct x86_emulate_ctxt *ctxt)
 static void sanitize_input(struct x86_emulate_ctxt *ctxt)
 {
     struct fuzz_state *s = ctxt->data;
-    struct fuzz_corpus *c = s->corpus;
-    struct cpu_user_regs *regs = &c->regs;
-    unsigned long bitmap = c->options;
+    struct cpu_user_regs *regs = ctxt->regs;
+    unsigned long bitmap = s->options;
 
     /* Some hooks can't be disabled. */
-    c->options &= ~((1<<HOOK_read)|(1<<HOOK_insn_fetch));
+    s->options &= ~((1<<HOOK_read)|(1<<HOOK_insn_fetch));
 
     /* Zero 'private' entries */
     regs->error_code = 0;
@@ -779,8 +779,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 ( c->cr[0] & X86_CR0_PG )
-        c->cr[0] |= X86_CR0_PE;
+    if ( s->cr[0] & X86_CR0_PG )
+        s->cr[0] |= X86_CR0_PE;
 
     /* EFLAGS.VM not available in long mode */
     if ( long_mode_active(ctxt) )
@@ -789,8 +789,8 @@ static void sanitize_input(struct x86_emulate_ctxt *ctxt)
     /* EFLAGS.VM implies 16-bit mode */
     if ( regs->rflags & X86_EFLAGS_VM )
     {
-        c->segments[x86_seg_cs].db = 0;
-        c->segments[x86_seg_ss].db = 0;
+        s->segments[x86_seg_cs].db = 0;
+        s->segments[x86_seg_ss].db = 0;
     }
 }
 
@@ -812,7 +812,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
     };
     struct x86_emulate_ctxt ctxt = {
         .data = &state,
-        .regs = &input.regs,
+        .regs = &state.regs,
         .addr_size = 8 * sizeof(void *),
         .sp_size = 8 * sizeof(void *),
     };
@@ -836,7 +836,9 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
     memcpy(&input, data_p, size);
 
     state.corpus = &input;
-    state.data_num = size - DATA_OFFSET;
+    state.data_num = size;
+
+    setup_state(&ctxt);
 
     sanitize_input(&ctxt);
 
-- 
2.14.1


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

[PATCH v2 10/13] fuzz/x86_emulate: Make input more compact

[George Dunlap]

At the moment, AFL reckons that for any given input, 87% of it is
completely irrelevant: that is, it can change it as much as it wants
but have no impact on the result of the test; and yet it can't remove
it.

This is largely because we interpret the blob handed to us as a large
struct, including CR values, MSR values, segment registers, and a full
cpu_user_regs.

Instead, modify our interpretation to have a "set state" stanza at the
front.  Begin by reading a byte; if it is lower than a certain
threshold, set some state according to what byte it is, and repeat.
Continue until the byte is above a certain threshold.

This allows AFL to compact any given test case much smaller; to the
point where now it reckons there is not a single byte of the test file
which becomes irrelevant.  Testing have shown that this option both
allows AFL to reach coverage much faster, and to have a total coverage
higher than with the old format.

Make this an option (rather than a unilateral change) to enable
side-by-side performance comparison of the old and new formats.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2: Port over previous changes

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/afl-harness.c | 13 +++-
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c   | 94 ++++++++++++++++++++---
 2 files changed, 97 insertions(+), 10 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/afl-harness.c b/tools/fuzz/x86_instruction_emulator/afl-harness.c
index 669f698711..806f54d606 100644
--- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
+++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
@@ -4,6 +4,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <getopt.h>
+#include <stdbool.h>
 
 extern int LLVMFuzzerInitialize(int *argc, char ***argv);
 extern int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size);
@@ -12,6 +13,8 @@ extern unsigned int fuzz_minimal_input_size(void);
 #define INPUT_SIZE  4096
 static uint8_t input[INPUT_SIZE];
 
+extern bool opt_compact;
+
 int main(int argc, char **argv)
 {
     size_t size;
@@ -22,13 +25,17 @@ int main(int argc, char **argv)
     setbuf(stdin, NULL);
     setbuf(stdout, NULL);
 
+    opt_compact = true;
+
     while ( 1 )
     {
         enum {
             OPT_MIN_SIZE,
+            OPT_COMPACT,
         };
         static const struct option lopts[] = {
             { "min-input-size", no_argument, NULL, OPT_MIN_SIZE },
+            { "compact", required_argument, NULL, OPT_COMPACT },
             { 0, 0, 0, 0 }
         };
         int c = getopt_long_only(argc, argv, "", lopts, NULL);
@@ -43,8 +50,12 @@ int main(int argc, char **argv)
             exit(0);
             break;
 
+        case OPT_COMPACT:
+            opt_compact = atoi(optarg);
+            break;
+            
         case '?':
-            printf("Usage: %s $FILE [$FILE...] | [--min-input-size]\n", argv[0]);
+            printf("Usage: %s [--compact=0|1] $FILE [$FILE...] | [--min-input-size]\n", argv[0]);
             exit(-1);
             break;
 
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index c8a5507f8b..d99a50d12c 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -53,6 +53,15 @@ struct fuzz_state
 };
 #define DATA_OFFSET offsetof(struct fuzz_state, corpus)
 
+bool opt_compact;
+
+unsigned int fuzz_minimal_input_size(void)
+{
+    if ( opt_compact )
+        return sizeof(unsigned long) + 1;
+    else
+        return DATA_OFFSET + 1;
+}
 
 static inline bool input_available(struct fuzz_state *s, size_t size)
 {
@@ -647,9 +656,81 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
 {
     struct fuzz_state *s = ctxt->data;
 
-    /* Fuzz all of the state in one go */
-    if (!input_read(s, s, DATA_OFFSET))
-        exit(-1);
+    if ( !opt_compact )
+    {
+        /* Fuzz all of the state in one go */
+        if (!input_read(s, s, DATA_OFFSET))
+            exit(-1);
+        return;
+    }
+
+    /* Modify only select bits of state */
+
+    /* Always read 'options' */
+    if ( !input_read(s, &s->options, sizeof(s->options)) )
+        return;
+    
+    while(1) {
+        uint16_t offset;
+
+        /* Read 16 bits to decide what bit of state to modify */
+        if ( !input_read(s, &offset, sizeof(offset)) )
+            return;
+
+        /* 
+         * Then decide if it's "pointing to" different bits of the
+         * state 
+         */
+
+        /* cr[]? */
+        if ( offset < 5 )
+        {
+            if ( !input_read(s, s->cr + offset, sizeof(*s->cr)) )
+                return;
+            printf("Setting CR %d to %lx\n", offset, s->cr[offset]);
+            continue;
+        }
+        
+        offset -= 5;
+
+        /* msr[]? */
+        if ( offset < MSR_INDEX_MAX )
+        {
+            if ( !input_read(s, s->msr + offset, sizeof(*s->msr)) )
+                return;
+            printf("Setting MSR i%d (%x) to %lx\n", offset,
+                   msr_index[offset], s->msr[offset]);
+            continue;
+        }
+
+        offset -= MSR_INDEX_MAX;
+
+        /* segments[]? */
+        if ( offset < SEG_NUM )
+        {
+            if ( !input_read(s, s->segments + offset, sizeof(*s->segments)) )
+                return;
+            printf("Setting Segment %d\n", offset);
+            continue;
+            
+        }
+
+        offset -= SEG_NUM;
+
+        /* regs? */
+        if ( offset < sizeof(struct cpu_user_regs)
+             && offset + sizeof(uint64_t) <= sizeof(struct cpu_user_regs) )
+        {
+            if ( !input_read(s, ((char *)ctxt->regs) + offset, sizeof(uint64_t)) )
+                return;
+            printf("Setting cpu_user_regs offset %x\n", offset);
+            continue;
+        }
+
+        /* None of the above -- take that as "start emulating" */
+        
+        return;
+    }
 }
 
 #define CANONICALIZE(x)                                   \
@@ -821,7 +902,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
     /* Reset all global state variables */
     memset(&input, 0, sizeof(input));
 
-    if ( size <= DATA_OFFSET )
+    if ( size < fuzz_minimal_input_size() )
     {
         printf("Input too small\n");
         return 1;
@@ -858,11 +939,6 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
     return 0;
 }
 
-unsigned int fuzz_minimal_input_size(void)
-{
-    return DATA_OFFSET + 1;
-}
-
 /*
  * Local variables:
  * mode: C
-- 
2.14.1


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

[PATCH v2 11/13] fuzz/x86_emulate: Add --rerun option to try to track down instability

[George Dunlap]

Current stability numbers are not 100%.  In order to help track this
down, add a --rerun option which will run the same input twice,
resetting the state in between each run, and comparing the state
afterwards.  If the state differs, call abort().

This allows AFL to help the process of tracking down what state is not
being reset properly between runs by proving testcases that
demonstrate the behavior.

To do this:

- Move ctxt into struct fuzz-state to simplify handling

- Rather than copying the data into input, treat the data handed as
  immutable and point each "copy" to it

- Factor out various steps (setting up fuzz state, running an
  individual test) so that they can be efficiently run either once or
  twice (as necessary)

- Compare the states afterwards, printing what's different and calling
  abort() if anything is found.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2:
- Fix some coding style issues
- Port over previous changes

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/afl-harness.c |   9 +-
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c   | 188 ++++++++++++++++++----
 2 files changed, 165 insertions(+), 32 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/afl-harness.c b/tools/fuzz/x86_instruction_emulator/afl-harness.c
index 806f54d606..6b0f66f923 100644
--- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
+++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
@@ -14,6 +14,7 @@ extern unsigned int fuzz_minimal_input_size(void);
 static uint8_t input[INPUT_SIZE];
 
 extern bool opt_compact;
+extern bool opt_rerun;
 
 int main(int argc, char **argv)
 {
@@ -32,10 +33,12 @@ int main(int argc, char **argv)
         enum {
             OPT_MIN_SIZE,
             OPT_COMPACT,
+            OPT_RERUN,
         };
         static const struct option lopts[] = {
             { "min-input-size", no_argument, NULL, OPT_MIN_SIZE },
             { "compact", required_argument, NULL, OPT_COMPACT },
+            { "rerun", no_argument, NULL, OPT_RERUN },
             { 0, 0, 0, 0 }
         };
         int c = getopt_long_only(argc, argv, "", lopts, NULL);
@@ -54,8 +57,12 @@ int main(int argc, char **argv)
             opt_compact = atoi(optarg);
             break;
             
+        case OPT_RERUN:
+            opt_rerun = true;
+            break;
+
         case '?':
-            printf("Usage: %s [--compact=0|1] $FILE [$FILE...] | [--min-input-size]\n", argv[0]);
+            printf("Usage: %s [--compact=0|1] [--rerun] $FILE [$FILE...] | [--min-input-size]\n", argv[0]);
             exit(-1);
             break;
 
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index d99a50d12c..21d00b7416 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -40,7 +40,7 @@ struct fuzz_state
     struct cpu_user_regs regs;
 
     /* Fuzzer's input data. */
-    struct fuzz_corpus *corpus;
+    const struct fuzz_corpus *corpus;
 
     /* Real amount of data backing corpus->data[]. */
     size_t data_num;
@@ -50,6 +50,7 @@ struct fuzz_state
 
     /* Emulation ops, some of which are disabled based on corpus->options. */
     struct x86_emulate_ops ops;
+    struct x86_emulate_ctxt ctxt;
 };
 #define DATA_OFFSET offsetof(struct fuzz_state, corpus)
 
@@ -496,6 +497,12 @@ static int fuzz_read_msr(
     const struct fuzz_state *s = ctxt->data;
     unsigned int idx;
 
+    /* 
+     * NB at the moment dump_state() relies on the fact that this
+     * cannot fail.  If we add in fuzzed failures we'll have to handle
+     * that differently.
+     */
+    
     switch ( reg )
     {
     case MSR_TSC_AUX:
@@ -616,6 +623,7 @@ static void dump_state(struct x86_emulate_ctxt *ctxt)
 
     printf(" rip: %"PRIx64"\n", regs->rip);
 
+    /* read_msr() never fails at the moment */
     fuzz_read_msr(MSR_EFER, &val, ctxt);
     printf("EFER: %"PRIx64"\n", val);
 }
@@ -660,7 +668,10 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
     {
         /* Fuzz all of the state in one go */
         if (!input_read(s, s, DATA_OFFSET))
+        {
+            printf("Input size too small\n");
             exit(-1);
+        }
         return;
     }
 
@@ -789,9 +800,8 @@ enum {
         printf("Disabling hook "#h"\n");               \
     }
 
-static void disable_hooks(struct x86_emulate_ctxt *ctxt)
+static void disable_hooks(struct fuzz_state *s)
 {
-    struct fuzz_state *s = ctxt->data;
     unsigned long bitmap = s->options;
 
     /* See also sanitize_input, some hooks can't be disabled. */
@@ -839,7 +849,7 @@ static void disable_hooks(struct x86_emulate_ctxt *ctxt)
  *  - ...bases to below 1Mb, 16-byte aligned
  *  - ...selectors to (base >> 4)
  */
-static void sanitize_input(struct x86_emulate_ctxt *ctxt)
+static void sanitize_state(struct x86_emulate_ctxt *ctxt)
 {
     struct fuzz_state *s = ctxt->data;
     struct cpu_user_regs *regs = ctxt->regs;
@@ -886,21 +896,138 @@ int LLVMFuzzerInitialize(int *argc, char ***argv)
     return 0;
 }
 
-int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
+void setup_fuzz_state(struct fuzz_state *state, const uint8_t *data_p, size_t size)
 {
-    struct fuzz_state state = {
-        .ops = all_fuzzer_ops,
-    };
-    struct x86_emulate_ctxt ctxt = {
-        .data = &state,
-        .regs = &state.regs,
-        .addr_size = 8 * sizeof(void *),
-        .sp_size = 8 * sizeof(void *),
-    };
+    memset(state, 0, sizeof(*state));
+    state->corpus = (struct fuzz_corpus *)data_p;
+    state->data_num = size;
+}
+
+int runtest(struct fuzz_state *state) {
     int rc;
 
-    /* Reset all global state variables */
-    memset(&input, 0, sizeof(input));
+    struct x86_emulate_ctxt *ctxt = &state->ctxt;
+    
+    state->ops = all_fuzzer_ops;
+
+    ctxt->data = state;
+    ctxt->regs = &state->regs;
+    ctxt->addr_size = ctxt->sp_size = 8 * sizeof(void *);
+
+    setup_state(ctxt);
+
+    sanitize_state(ctxt);
+
+    disable_hooks(state);
+
+    do {
+        /* FIXME: Until we actually implement SIGFPE handling properly */
+        setup_fpu_exception_handler();
+
+        set_sizes(ctxt);
+        dump_state(ctxt);
+
+        rc = x86_emulate(ctxt, &state->ops);
+        printf("Emulation result: %d\n", rc);
+    } while ( rc == X86EMUL_OKAY );
+
+    return 0;
+}
+
+void compare_states(struct fuzz_state state[2])
+{
+    // First zero any "internal" pointers
+    state[0].corpus = state[1].corpus = NULL;
+    state[0].ctxt.data = state[1].ctxt.data = NULL;
+    state[0].ctxt.regs = state[1].ctxt.regs = NULL;
+
+    
+    if ( memcmp(&state[0], &state[1], sizeof(struct fuzz_state)) )
+    {
+        int i;
+
+        printf("State mismatch\n");
+
+        for ( i=0; i<5; i++)
+            if ( state[0].cr[i] != state[1].cr[i] )
+                printf("cr[%d]: %lx != %lx\n",
+                       i, state[0].cr[i], state[1].cr[i]);
+        
+        for ( i=0; i<MSR_INDEX_MAX; i++)
+            if ( state[0].msr[i] != state[1].msr[i] )
+                printf("msr[%d]: %lx != %lx\n",
+                       i, state[0].msr[i], state[1].msr[i]);
+        
+        for ( i=0; i<SEG_NUM; i++)
+            if ( memcmp(&state[0].segments[i], &state[1].segments[i],
+                        sizeof(state[0].segments[0])) )
+                printf("segments[%d] differ!\n", i);
+
+        if ( state[0].data_num != state[1].data_num )
+            printf("data_num: %lx !=  %lx\n", state[0].data_num,
+                   state[1].data_num);
+        if ( state[0].data_index != state[1].data_index )
+            printf("data_index: %lx !=  %lx\n", state[0].data_index,
+                   state[1].data_index);
+
+        if ( memcmp(&state[0].regs, &state[1].regs, sizeof(state[0].regs)) )
+        {
+            printf("registers differ!\n");
+            /* Print If Not Equal */
+#define PINE(elem)\
+            if ( state[0].elem != state[1].elem ) \
+                printf(#elem " differ: %lx != %lx\n", \
+                       (unsigned long)state[0].elem, \
+                       (unsigned long)state[1].elem)
+            PINE(regs.r15);
+            PINE(regs.r14);
+            PINE(regs.r13);
+            PINE(regs.r12);
+            PINE(regs.rbp);
+            PINE(regs.rbx);
+            PINE(regs.r10);
+            PINE(regs.r11);
+            PINE(regs.r9);
+            PINE(regs.r8);
+            PINE(regs.rax);
+            PINE(regs.rcx);
+            PINE(regs.rdx);
+            PINE(regs.rsi);
+            PINE(regs.rdi);
+
+            for ( i = offsetof(struct cpu_user_regs, error_code) / sizeof(unsigned);
+                  i < sizeof(state[1].regs)/sizeof(unsigned); i++ )
+            {
+                printf("[%04lu] %08x %08x\n",
+                        i * sizeof(unsigned), ((unsigned *)&state[0].regs)[i],
+                       ((unsigned *)&state[1].regs)[i]);
+            }
+        }
+
+        if ( memcmp(&state[0].ops, &state[1].ops, sizeof(state[0].ops)) )
+            printf("ops differ!\n");
+
+        if ( memcmp(&state[0].ctxt, &state[1].ctxt, sizeof(state[0].ctxt)) )
+        {
+            printf("ctxt differs!\n");
+            for ( i = 0;  i < sizeof(state[0].ctxt)/sizeof(unsigned); i++ )
+            {
+                printf("[%04lu] %08x %08x\n",
+                        i * sizeof(unsigned), ((unsigned *)&state[0].ctxt)[i],
+                       ((unsigned *)&state[1].ctxt)[i]);
+            }
+            
+        }
+
+        abort();
+    }
+}
+
+bool opt_rerun = false;
+
+int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
+{
+    struct fuzz_state state[2];
 
     if ( size < fuzz_minimal_input_size() )
     {
@@ -908,7 +1035,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
         return 1;
     }
 
-    if ( size > sizeof(input) )
+    if ( size > sizeof(struct fuzz_corpus) )
     {
         printf("Input too large\n");
         return 1;
@@ -916,25 +1043,24 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
 
     memcpy(&input, data_p, size);
 
-    state.corpus = &input;
-    state.data_num = size;
-
-    setup_state(&ctxt);
+    setup_fuzz_state(&state[0], data_p, size);
+    
+    if ( opt_rerun )
+        printf("||| INITIAL RUN |||\n");
+    
+    runtest(&state[0]);
 
-    sanitize_input(&ctxt);
+    if ( !opt_rerun )
+        return 0;
 
-    disable_hooks(&ctxt);
+    /* Reset all global state variables again */
+    setup_fuzz_state(&state[1], data_p, size);
 
-    do {
-        /* FIXME: Until we actually implement SIGFPE handling properly */
-        setup_fpu_exception_handler();
+    printf("||| SECOND RUN |||\n");
 
-        set_sizes(&ctxt);
-        dump_state(&ctxt);
+    runtest(&state[1]);
 
-        rc = x86_emulate(&ctxt, &state.ops);
-        printf("Emulation result: %d\n", rc);
-    } while ( rc == X86EMUL_OKAY );
+    compare_states(state);
 
     return 0;
 }
-- 
2.14.1


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

[PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state

[George Dunlap]

x86_emulate() operates not only on state passed to it in
cpu_user_regs, but also on state currently found on the cpu: namely,
the FPU and XMM registers.  At the moment, we re-zero (and/or
re-initialize) cpu_user_regs on every invocation, but leave the
cpu-stored state alone.  In "persistent mode", this causes test cases
to behave differently -- sometimes significantly so -- depending on
which test cases have been run beforehand.

Zero out the state before each test run, and then fuzz it based on the
corpus input.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
v2: Rebase on top of previous changes

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 71 +++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 21d00b7416..48cad0307a 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -38,6 +38,8 @@ struct fuzz_state
     uint64_t msr[MSR_INDEX_MAX];
     struct segment_register segments[SEG_NUM];
     struct cpu_user_regs regs;
+    char fxsave[512] __attribute__((aligned(16)));
+
 
     /* Fuzzer's input data. */
     const struct fuzz_corpus *corpus;
@@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
 };
 #undef SET
 
+static void _set_fpu_state(char *fxsave, bool store)
+{
+    if ( cpu_has_fxsr )
+    {
+        static union __attribute__((__aligned__(16))) {
+            char x[464];
+            struct {
+                uint32_t other[6];
+                uint32_t mxcsr;
+                uint32_t mxcsr_mask;
+                /* ... */
+            };
+        } *fxs;
+
+        fxs = (typeof(fxs)) fxsave;
+
+        if ( store ) {
+            char null[512] __attribute__((aligned(16))) = { 0 };
+            asm volatile(" fxrstor %0; "::"m"(*null));
+            asm volatile(" fxrstor %0; "::"m"(*fxsave));
+        }
+        
+        asm volatile( "fxsave %0" : "=m" (*fxs) );
+
+        if ( fxs->mxcsr_mask )
+            mxcsr_mask = fxs->mxcsr_mask;
+        else
+            mxcsr_mask = 0x000ffbf;
+    }
+}
+
+static void set_fpu_state(char *fxsave)
+{
+    _set_fpu_state(fxsave, true);
+}
+
+static void save_fpu_state(char *fxsave)
+{
+    _set_fpu_state(fxsave, false);
+}
+
 static void setup_fpu_exception_handler(void)
 {
     /* FIXME - just disable exceptions for now */
@@ -737,6 +780,17 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
             printf("Setting cpu_user_regs offset %x\n", offset);
             continue;
         }
+        offset -= sizeof(struct cpu_user_regs);
+
+        /* Fuzz fxsave state */
+        if ( offset < 128 )
+        {
+            if ( !input_read(s, s->fxsave + (offset * 4), 4) )
+                return;
+            printf("Setting fxsave offset %x\n", offset * 4);
+            continue;
+        }
+        offset -= 128;
 
         /* None of the above -- take that as "start emulating" */
         
@@ -883,6 +937,9 @@ static void sanitize_state(struct x86_emulate_ctxt *ctxt)
         s->segments[x86_seg_cs].db = 0;
         s->segments[x86_seg_ss].db = 0;
     }
+
+    /* Setting this value seems to cause crashes in fxrstor */
+    *((unsigned int *)(s->fxsave) + 6) = 0;
 }
 
 int LLVMFuzzerInitialize(int *argc, char ***argv)
@@ -920,6 +977,8 @@ int runtest(struct fuzz_state *state) {
 
     disable_hooks(state);
 
+    set_fpu_state(state->fxsave);
+
     do {
         /* FIXME: Until we actually implement SIGFPE handling properly */
         setup_fpu_exception_handler();
@@ -931,6 +990,8 @@ int runtest(struct fuzz_state *state) {
         printf("Emulation result: %d\n", rc);
     } while ( rc == X86EMUL_OKAY );
 
+    save_fpu_state(state->fxsave);
+    
     return 0;
 }
 
@@ -1007,6 +1068,16 @@ void compare_states(struct fuzz_state state[2])
         if ( memcmp(&state[0].ops, &state[1].ops, sizeof(state[0].ops)) )
             printf("ops differ!\n");
 
+        if ( memcmp(&state[0].fxsave, &state[1].fxsave, sizeof(state[0].fxsave)) )
+        {
+            printf("fxsave differs!\n");
+            for ( i = 0;  i < sizeof(state[0].fxsave)/sizeof(unsigned); i++ )
+            {
+                printf("[%04lu] %08x %08x\n",
+                        i * sizeof(unsigned), ((unsigned *)&state[0].fxsave)[i], ((unsigned *)&state[1].fxsave)[i]);
+            }
+        }
+
         if ( memcmp(&state[0].ctxt, &state[1].ctxt, sizeof(state[0].ctxt)) )
         {
             printf("ctxt differs!\n");
-- 
2.14.1


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

[PATCH v2 13/13] fuzz/x86_emulate: Add an option to limit the number of instructions executed

[George Dunlap]

AFL considers a testcase to be a useful addition not only if there are
tuples exercised by that testcase which were not exercised otherwise,
but also if the *number* of times an individual tuple is exercised
changes significantly; in particular, if the number of the highes bit
changes (i.e., if it is run 1, 2-3, 4-7, 8-15, &c).

Unfortunately, one simple way to increase these stats it to execute
the same (or similar) instructions multiple times.  Such long
testcases take exponentially longer to fuzz: the fuzzer spends more
time flipping bits looking for meaningful changes, and each execution
takes longer because it is doing more things.  So long paths which add
nothing to the actual code coverage but effectively "distract" the
fuzzer, making it less effective.

Experiments have shown that not allowing infinite number of
instruction retries for the old (non-compact) format does indeed speed
up and increase code coverage.  However, it has also shown that on the
new, more compact format, having no instruction limit causes the highest
throughput in code coverage.

So leave the option in, but have it default to 0 (no limit).

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/afl-harness.c | 9 ++++++++-
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c   | 7 ++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/afl-harness.c b/tools/fuzz/x86_instruction_emulator/afl-harness.c
index 6b0f66f923..db6bb2891f 100644
--- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
+++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
@@ -15,6 +15,7 @@ static uint8_t input[INPUT_SIZE];
 
 extern bool opt_compact;
 extern bool opt_rerun;
+extern int opt_instruction_limit;
 
 int main(int argc, char **argv)
 {
@@ -34,11 +35,13 @@ int main(int argc, char **argv)
             OPT_MIN_SIZE,
             OPT_COMPACT,
             OPT_RERUN,
+            OPT_INSTRUCTION_LIMIT,
         };
         static const struct option lopts[] = {
             { "min-input-size", no_argument, NULL, OPT_MIN_SIZE },
             { "compact", required_argument, NULL, OPT_COMPACT },
             { "rerun", no_argument, NULL, OPT_RERUN },
+            { "instruction-limit", required_argument, NULL, OPT_INSTRUCTION_LIMIT },
             { 0, 0, 0, 0 }
         };
         int c = getopt_long_only(argc, argv, "", lopts, NULL);
@@ -61,8 +64,12 @@ int main(int argc, char **argv)
             opt_rerun = true;
             break;
 
+        case OPT_INSTRUCTION_LIMIT:
+            opt_instruction_limit = atoi(optarg);
+            break;
+
         case '?':
-            printf("Usage: %s [--compact=0|1] [--rerun] $FILE [$FILE...] | [--min-input-size]\n", argv[0]);
+            printf("Usage: %s [--compact=0|1] [--rerun] [--instruction-limit=N] $FILE [$FILE...] | [--min-input-size]\n", argv[0]);
             exit(-1);
             break;
 
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 48cad0307a..c2ab029347 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -960,10 +960,13 @@ void setup_fuzz_state(struct fuzz_state *state, const uint8_t *data_p, size_t si
     state->data_num = size;
 }
 
+int opt_instruction_limit = 0;
+
 int runtest(struct fuzz_state *state) {
     int rc;
 
     struct x86_emulate_ctxt *ctxt = &state->ctxt;
+    int icount = 0;
     
     state->ops = all_fuzzer_ops;
 
@@ -988,7 +991,9 @@ int runtest(struct fuzz_state *state) {
 
         rc = x86_emulate(ctxt, &state->ops);
         printf("Emulation result: %d\n", rc);
-    } while ( rc == X86EMUL_OKAY );
+    } while ( rc == X86EMUL_OKAY &&
+              (!opt_instruction_limit ||
+               (++icount < opt_instruction_limit)) );
 
     save_fpu_state(state->fxsave);
     
-- 
2.14.1


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

Re: [PATCH v2 02/13] fuzz/x86_emulate: Actually use cpu_regs input

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> Commit c07574b reorganized the way fuzzing was done, explicitly
> creating a structure that the input data would be copied into.
> 
> Unfortunately, the cpu register state used by the emulator is on the
> stack; it's cleared, but data is never copied into it.
> 
> If we're explicitly setting an entirely new cpu_regs struct for each
> new input anyway, there's no need to have two copies around anymore;
> just point to the one in the data structure.
> 
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> Reviewed-by: Wei Liu <wei.liu2@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 v2 03/13] fuzz/x86_emulate: Clear errors after each iteration

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> Once feof() returns true for a stream, it will continue to return true
> for that stream until clearerr() is called (or the stream is closed
> and re-opened).
> 
> In llvm-clang-fast-mode, the same file descriptor is used for each
> iteration of the loop, meaning that the "Input too large" check was
> broken -- feof() would return true even if the fread() hadn't hit the
> end of the file.  The result is that AFL generates testcases of
> arbitrary size.
> 
> Fix this by clearing the error after each iteration.
> 
> Signed-off-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

Re: [PATCH v2 04/13] fuzz/x86_emulate: Improve failure descriptions in x86_emulate harness

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -52,6 +52,14 @@ struct fuzz_state
>      struct x86_emulate_ops ops;
>  };
>  
> +char *x86emul_return_string[] = {

With "static const char* const"
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 v2 05/13] fuzz/x86_emulate: Implement input_read() and input_avail()

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -52,6 +52,22 @@ struct fuzz_state
>      struct x86_emulate_ops ops;
>  };
>  
> +static inline bool input_available(struct fuzz_state *s, size_t size)

s can be pointer to const

Also how about shortening the function name to what the title says?

> +{
> +    return s->data_index + size < s->data_num;

Shouldn't this be <= ?

> +}
> +
> +static inline bool input_read(struct fuzz_state *s, void *dst, size_t size)
> +{
> +    if ( !input_available(s, size) )
> +        return 0;

false

> +
> +    memcpy(dst, &s->corpus->data[s->data_index], size);
> +    s->data_index += size;
> +
> +    return 1;

true

Jan


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

Re: [PATCH v2 06/13] fuzz/x86_emulate: Rename the file containing the wrapper code

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> --- a/tools/fuzz/x86_instruction_emulator/Makefile
> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
> @@ -18,22 +18,22 @@ asm:
>  
>  asm/%: asm ;
>  
> -x86_emulate.c x86_emulate.h: %:
> +x86_emulate_user.c x86_emulate_user.h: %:

How about avoiding the names getting even longer? E.g. using
x86-emulate.[ch] or x86emul-user.[ch] instead?

> @@ -42,7 +42,7 @@ all: x86-insn-fuzz-all
>  
>  .PHONY: distclean
>  distclean: clean
> -	rm -f x86_emulate x86_emulate.c x86_emulate.h asm
> +	rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm

If you want to stick to the longer names, would you mind taking the
opportunity to make this just x86_emulate* ?

Jan


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

Re: [PATCH v2 07/13] fuzz/x86_emulate: Add 'afl-cov' target

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> --- a/tools/fuzz/README.afl
> +++ b/tools/fuzz/README.afl
> @@ -41,3 +41,17 @@ Use the x86 instruction emulator fuzzer as an example.
>     $ $AFLPATH/afl-fuzz -t 1000 -i testcase_dir -o findings_dir -- ./afl-harness
>  
>  Please see AFL documentation for more information.
> +
> +# GENERATING COVERAGE INFORMATION
> +
> +To use afl-cov or gcov, you need a separate binary instrumented to
> +generate coverage data.  To do this, use the target `afl-cov`:
> +
> +    $ make afl-cov #produces afl-harness-cov
> +
> +NOTE: Please also note that the coverage instrumentation hard-codes
> +the absolute path for the instrumentation read and write files in the
> +binary; so coverage data will always show up in the build directory no
> +matter where you run the binary from.
> +
> +Please see afl-cov and/or gcov documentation for more information.
> \ No newline at end of file

Would you please add the missing newline?

> --- a/tools/fuzz/x86_instruction_emulator/Makefile
> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
> @@ -23,19 +23,34 @@ x86_emulate_user.c x86_emulate_user.h: %:
>  
>  CFLAGS += $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -I.
>  
> +GCOV_FLAGS=--coverage

:= ?

>  x86.h := asm/x86-vendors.h asm/x86-defns.h asm/msr-index.h
>  x86_emulate.h := x86_emulate_user.h x86_emulate/x86_emulate.h $(x86.h)
>  
> -x86_emulate_user.o: x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
> +X86_EMULATE_INPUTS = x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
> +x86_emulate_user.o: $(X86_EMULATE_INPUTS)
> +
> +x86_emulate_user-cov.o: $(X86_EMULATE_INPUTS)
> +	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) -o $@ x86_emulate_user.c
>  
>  fuzz-emul.o: $(x86_emulate.h)
>  
> +fuzz-emul-cov.o: fuzz-emul.c $(x86_emulate.h)
> +	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) -o $@ fuzz-emul.c
> +
> +afl-harness-cov.o: afl-harness.c
> +	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) $^ -o $@

Rather than effectively repeating this command three time, I think
someone else had already suggested to use a pattern rule instead.

> @@ -46,7 +61,7 @@ distclean: clean
>  
>  .PHONY: clean
>  clean:
> -	rm -f *.a *.o .*.d afl-harness
> +	rm -f *.a *.o .*.d afl-harness afl-harness-cov *.gcda *.gcno *.gcov

Perhaps simply *.gc* to cover for possible future generated file types?

Jan


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

Re: [PATCH v2 08/13] fuzz/x86_emulate: Take multiple test files for inputs

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
> @@ -16,6 +16,8 @@ int main(int argc, char **argv)
>  {
>      size_t size;
>      FILE *fp = NULL;
> +    int count = 0;
> +    int max;

Generally speaking these should be unsigned int, but I see how this
collides with the types of the variables max is being calculated from.
In any event both could go on a single line.

> @@ -66,11 +70,14 @@ int main(int argc, char **argv)
>      __AFL_INIT();
>  
>      while ( __AFL_LOOP(1000) )
> +#else
> +    for( count = 0; count < max; count++ )

Initially I've thought the initializer on count was pointless further
up because of the re-initialization here. Of course that's needed
because of the #if/#else this sits in. Hence I wonder whether omitting
the assignment here wouldn't be appropriate - it wouldn't really be
wromng for a compiler to warn about this redundancy.

Either way
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 v2 09/13] fuzz/x86_emulate: Move all state into fuzz_state

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> @@ -39,6 +33,12 @@ struct fuzz_corpus
>   */
>  struct fuzz_state
>  {
> +    unsigned long options;
> +    unsigned long cr[5];
> +    uint64_t msr[MSR_INDEX_MAX];
> +    struct segment_register segments[SEG_NUM];
> +    struct cpu_user_regs regs;
> +
>      /* Fuzzer's input data. */
>      struct fuzz_corpus *corpus;
>  
> @@ -51,6 +51,8 @@ struct fuzz_state
>      /* Emulation ops, some of which are disabled based on corpus->options. */
>      struct x86_emulate_ops ops;
>  };
> +#define DATA_OFFSET offsetof(struct fuzz_state, corpus)
> +
>  

Personally I think this would better be placed right between the two
respective fields in the structure, for it to at the same time serve as
a clear indication that it needs either changing when a field would be
inserted there, or the insertion be done elsewhere. Also please don't
add another blank line here.

> @@ -760,12 +761,11 @@ static void disable_hooks(struct x86_emulate_ctxt *ctxt)
>  static void sanitize_input(struct x86_emulate_ctxt *ctxt)
>  {
>      struct fuzz_state *s = ctxt->data;
> -    struct fuzz_corpus *c = s->corpus;
> -    struct cpu_user_regs *regs = &c->regs;
> -    unsigned long bitmap = c->options;
> +    struct cpu_user_regs *regs = ctxt->regs;

I think this would more obviously look like the equivalent of the old
code when being &s->regs, but the net effect is the same afaict, so it
doesn't really matter.

In any event (with at least the extra blank line removed)
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 v2 10/13] fuzz/x86_emulate: Make input more compact

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> @@ -22,13 +25,17 @@ int main(int argc, char **argv)
>      setbuf(stdin, NULL);
>      setbuf(stdout, NULL);
>  
> +    opt_compact = true;

How about giving the variable an initializer instead?

> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -53,6 +53,15 @@ struct fuzz_state
>  };
>  #define DATA_OFFSET offsetof(struct fuzz_state, corpus)
>  
> +bool opt_compact;
> +
> +unsigned int fuzz_minimal_input_size(void)
> +{
> +    if ( opt_compact )
> +        return sizeof(unsigned long) + 1;

What is this value choice based on / derived from? Oh, judging from
code further down it may be one more than the size of the options
field, in which case it should be sizeof(...->options) here.

> +    else

I'd prefer if an else like this one was dropped.

> @@ -647,9 +656,81 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
>  {
>      struct fuzz_state *s = ctxt->data;
>  
> -    /* Fuzz all of the state in one go */
> -    if (!input_read(s, s, DATA_OFFSET))
> -        exit(-1);
> +    if ( !opt_compact )
> +    {
> +        /* Fuzz all of the state in one go */
> +        if (!input_read(s, s, DATA_OFFSET))

Missing blanks.

> +            exit(-1);
> +        return;
> +    }
> +
> +    /* Modify only select bits of state */
> +
> +    /* Always read 'options' */
> +    if ( !input_read(s, &s->options, sizeof(s->options)) )
> +        return;
> +    
> +    while(1) {

Style. And for compatibility (read: no warnings) with as wide a range
of compilers as possible, generally for ( ; ; ) is better to use.

> +        uint16_t offset;
> +
> +        /* Read 16 bits to decide what bit of state to modify */
> +        if ( !input_read(s, &offset, sizeof(offset)) )
> +            return;

Doesn't this suggest minimal input size wants to be one higher than
what you currently enforce? And isn't the use of uint16_t here in
conflict with the description talking about reading a byte every time?

> +        /* 
> +         * Then decide if it's "pointing to" different bits of the
> +         * state 
> +         */
> +
> +        /* cr[]? */
> +        if ( offset < 5 )

ARRAY_SIZE()

> +        {
> +            if ( !input_read(s, s->cr + offset, sizeof(*s->cr)) )
> +                return;
> +            printf("Setting CR %d to %lx\n", offset, s->cr[offset]);
> +            continue;
> +        }
> +        
> +        offset -= 5;

Same here then.

> +        /* msr[]? */
> +        if ( offset < MSR_INDEX_MAX )

Even here (and below) use of ARRAY_SIZE() may be better.

> +        {
> +            if ( !input_read(s, s->msr + offset, sizeof(*s->msr)) )
> +                return;
> +            printf("Setting MSR i%d (%x) to %lx\n", offset,
> +                   msr_index[offset], s->msr[offset]);
> +            continue;
> +        }
> +
> +        offset -= MSR_INDEX_MAX;
> +
> +        /* segments[]? */
> +        if ( offset < SEG_NUM )
> +        {
> +            if ( !input_read(s, s->segments + offset, sizeof(*s->segments)) )
> +                return;
> +            printf("Setting Segment %d\n", offset);
> +            continue;
> +            
> +        }
> +
> +        offset -= SEG_NUM;
> +
> +        /* regs? */
> +        if ( offset < sizeof(struct cpu_user_regs)
> +             && offset + sizeof(uint64_t) <= sizeof(struct cpu_user_regs) )
> +        {
> +            if ( !input_read(s, ((char *)ctxt->regs) + offset, sizeof(uint64_t)) )
> +                return;
> +            printf("Setting cpu_user_regs offset %x\n", offset);
> +            continue;
> +        }
> +
> +        /* None of the above -- take that as "start emulating" */
> +        
> +        return;
> +    }

Having come here I wonder whether the use of "byte" in the description
is right, and you mean "uint8_t offset" above, as you're far from
consuming the entire 256 value range.

Additionally, was the order of elements here chosen for any specific
reason? It would seem to me that elements having a more significant
effect on emulation may be worth filling first, and I'm not convinced
the "all CRs, all MSRs, all SREGs, all GPRs" order matches that.

Jan

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

Re: [PATCH v2 11/13] fuzz/x86_emulate: Add --rerun option to try to track down instability

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
> @@ -14,6 +14,7 @@ extern unsigned int fuzz_minimal_input_size(void);
>  static uint8_t input[INPUT_SIZE];
>  
>  extern bool opt_compact;
> +extern bool opt_rerun;

Seeing a second such variable appear, I think it would really be better
to introduce a local header, included by both producer and consumer.

> @@ -886,21 +896,138 @@ int LLVMFuzzerInitialize(int *argc, char ***argv)
>      return 0;
>  }
>  
> -int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
> +void setup_fuzz_state(struct fuzz_state *state, const uint8_t *data_p, size_t size)

static (also for other new helper functions)?

>  {
> -    struct fuzz_state state = {
> -        .ops = all_fuzzer_ops,
> -    };
> -    struct x86_emulate_ctxt ctxt = {
> -        .data = &state,
> -        .regs = &state.regs,
> -        .addr_size = 8 * sizeof(void *),
> -        .sp_size = 8 * sizeof(void *),
> -    };
> +    memset(state, 0, sizeof(*state));
> +    state->corpus = (struct fuzz_corpus *)data_p;

Please don't cast away constness here. Perhaps best to make the parameter
const void *, allowing for the cast to be dropped altogether.

> +int runtest(struct fuzz_state *state) {
>      int rc;
>  
> -    /* Reset all global state variables */
> -    memset(&input, 0, sizeof(input));
> +    struct x86_emulate_ctxt *ctxt = &state->ctxt;
> +    
> +    state->ops = all_fuzzer_ops;
> +
> +    ctxt->data = state;
> +    ctxt->regs = &state->regs;
> +    ctxt->addr_size = ctxt->sp_size = 8 * sizeof(void *);

Is this actually necessary? I don't see a way for set_sizes() to be
bypassed.

> +void compare_states(struct fuzz_state state[2])
> +{
> +    // First zero any "internal" pointers
> +    state[0].corpus = state[1].corpus = NULL;
> +    state[0].ctxt.data = state[1].ctxt.data = NULL;
> +    state[0].ctxt.regs = state[1].ctxt.regs = NULL;
> +
> +    

No double blank lines please.

> +    if ( memcmp(&state[0], &state[1], sizeof(struct fuzz_state)) )
> +    {
> +        int i;

unsigned int (and then %u in the format strings below)

> +        printf("State mismatch\n");
> +
> +        for ( i=0; i<5; i++)

Blanks missing and please use ARRAY_SIZE() again (also further down).

> +            if ( state[0].cr[i] != state[1].cr[i] )
> +                printf("cr[%d]: %lx != %lx\n",
> +                       i, state[0].cr[i], state[1].cr[i]);
> +        
> +        for ( i=0; i<MSR_INDEX_MAX; i++)
> +            if ( state[0].msr[i] != state[1].msr[i] )
> +                printf("msr[%d]: %lx != %lx\n",
> +                       i, state[0].msr[i], state[1].msr[i]);
> +        
> +        for ( i=0; i<SEG_NUM; i++)
> +            if ( memcmp(&state[0].segments[i], &state[1].segments[i],
> +                        sizeof(state[0].segments[0])) )
> +                printf("segments[%d] differ!\n", i);

The actual values would likely be helpful to be printed here, just like
you do for all other state elements.

> +        if ( state[0].data_num != state[1].data_num )
> +            printf("data_num: %lx !=  %lx\n", state[0].data_num,
> +                   state[1].data_num);
> +        if ( state[0].data_index != state[1].data_index )
> +            printf("data_index: %lx !=  %lx\n", state[0].data_index,
> +                   state[1].data_index);
> +
> +        if ( memcmp(&state[0].regs, &state[1].regs, sizeof(state[0].regs)) )
> +        {
> +            printf("registers differ!\n");
> +            /* Print If Not Equal */
> +#define PINE(elem)\
> +            if ( state[0].elem != state[1].elem ) \
> +                printf(#elem " differ: %lx != %lx\n", \
> +                       (unsigned long)state[0].elem, \
> +                       (unsigned long)state[1].elem)
> +            PINE(regs.r15);
> +            PINE(regs.r14);
> +            PINE(regs.r13);
> +            PINE(regs.r12);
> +            PINE(regs.rbp);
> +            PINE(regs.rbx);
> +            PINE(regs.r10);
> +            PINE(regs.r11);
> +            PINE(regs.r9);
> +            PINE(regs.r8);
> +            PINE(regs.rax);
> +            PINE(regs.rcx);
> +            PINE(regs.rdx);
> +            PINE(regs.rsi);
> +            PINE(regs.rdi);
> +
> +            for ( i = offsetof(struct cpu_user_regs, error_code) / sizeof(unsigned);
> +                  i < sizeof(state[1].regs)/sizeof(unsigned); i++ )
> +            {
> +                printf("[%04lu] %08x %08x\n",

I think this wants to be %04zu (or perhaps better %4zu or %04zx). Same
for ctxt printing further down.

> @@ -908,7 +1035,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>          return 1;
>      }
>  
> -    if ( size > sizeof(input) )
> +    if ( size > sizeof(struct fuzz_corpus) )

What's the difference between the two variants?

> @@ -916,25 +1043,24 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>  
>      memcpy(&input, data_p, size);
>  
> -    state.corpus = &input;
> -    state.data_num = size;
> -
> -    setup_state(&ctxt);
> +    setup_fuzz_state(&state[0], data_p, size);
> +    
> +    if ( opt_rerun )
> +        printf("||| INITIAL RUN |||\n");
> +    
> +    runtest(&state[0]);
>  
> -    sanitize_input(&ctxt);
> +    if ( !opt_rerun )
> +        return 0;

Could I talk you into inverting the condition such that there'll be
only a single "return 0" at the end of the function?

And then - has this patch actually helped pinpoint any problems? The
ones deaslt with by the next patch perhaps?

Jan

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

Re: [PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>  };
>  #undef SET
>  
> +static void _set_fpu_state(char *fxsave, bool store)
> +{
> +    if ( cpu_has_fxsr )
> +    {
> +        static union __attribute__((__aligned__(16))) {
> +            char x[464];

The final part of the save area isn't being written, yes, but is it
really worth saving the few bytes of stack space here, rather than
having the expected 512 as array dimension?

> +            struct {
> +                uint32_t other[6];
> +                uint32_t mxcsr;
> +                uint32_t mxcsr_mask;
> +                /* ... */
> +            };
> +        } *fxs;
> +
> +        fxs = (typeof(fxs)) fxsave;
> +
> +        if ( store ) {

Style.

> +            char null[512] __attribute__((aligned(16))) = { 0 };

No need for the 0 and a blank line between declaration and statements
please.

> +            asm volatile(" fxrstor %0; "::"m"(*null));
> +            asm volatile(" fxrstor %0; "::"m"(*fxsave));

Style again - these want to follow the

    asm volatile ( "..." :: "m" (...) )

form. No need for the ; following the instructions.

> +        }
> +        
> +        asm volatile( "fxsave %0" : "=m" (*fxs) );

This is pretty confusing, the more with the different variable names
used which point to the same piece of memory. You basically store back
into the area you've read from. Is the caller expecting the memory area
to change? Is this being done other than for convenience to not have
another instance of scratch space on the stack? Some comment on the
intentions may be helpful here.

The function's parameter name being "store" adds to the confusion,
since what it controls is actually what we call "load" on x86 (or
"restore" following the insn mnemonics).

And then - what about YMM register state? Other more exotic registers
(like the BND* ones) may indeed not be that relevant to fuzz yet.

> @@ -737,6 +780,17 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
>              printf("Setting cpu_user_regs offset %x\n", offset);
>              continue;
>          }
> +        offset -= sizeof(struct cpu_user_regs);
> +
> +        /* Fuzz fxsave state */
> +        if ( offset < 128 )
> +        {
> +            if ( !input_read(s, s->fxsave + (offset * 4), 4) )
> +                return;
> +            printf("Setting fxsave offset %x\n", offset * 4);

What's this 32-bit granularity derived from?

> @@ -883,6 +937,9 @@ static void sanitize_state(struct x86_emulate_ctxt *ctxt)
>          s->segments[x86_seg_cs].db = 0;
>          s->segments[x86_seg_ss].db = 0;
>      }
> +
> +    /* Setting this value seems to cause crashes in fxrstor */
> +    *((unsigned int *)(s->fxsave) + 6) = 0;

That's the MXCSR field - instead of storing zero you want to mask with
mxcsr_mask. To avoid the ugly literal 6 (and to make clear what it is
that needs adjustment here) it may also be worthwhile widening the
scope of the type declared in _set_fpu_state() and use it for struct
fuzz_state's fxsave field.

Jan


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

Re: [PATCH v2 13/13] fuzz/x86_emulate: Add an option to limit the number of instructions executed

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> AFL considers a testcase to be a useful addition not only if there are
> tuples exercised by that testcase which were not exercised otherwise,
> but also if the *number* of times an individual tuple is exercised
> changes significantly; in particular, if the number of the highes bit
> changes (i.e., if it is run 1, 2-3, 4-7, 8-15, &c).

Perhaps I simply don't know about AFL (yet) to understand how "highest
bit" matters here, or even whose highest bits there's talk of.

> Unfortunately, one simple way to increase these stats it to execute
> the same (or similar) instructions multiple times.

But the change here doesn't look at instruction similarity at all.

> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -960,10 +960,13 @@ void setup_fuzz_state(struct fuzz_state *state, const uint8_t *data_p, size_t si
>      state->data_num = size;
>  }
>  
> +int opt_instruction_limit = 0;

unsigned int (and formally no need for an initializer)

>  int runtest(struct fuzz_state *state) {
>      int rc;
>  
>      struct x86_emulate_ctxt *ctxt = &state->ctxt;
> +    int icount = 0;

unsigned int

> @@ -988,7 +991,9 @@ int runtest(struct fuzz_state *state) {
>  
>          rc = x86_emulate(ctxt, &state->ops);
>          printf("Emulation result: %d\n", rc);
> -    } while ( rc == X86EMUL_OKAY );
> +    } while ( rc == X86EMUL_OKAY &&
> +              (!opt_instruction_limit ||
> +               (++icount < opt_instruction_limit)) );

Hmm, if the initalizer of opt_instruction_limit was UINT_MAX, I think
this wouldn't severely impact results (running 4 billion emulations is
simply going to take too long) and this expression could be a simple
comparison.

Jan


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

Re: [PATCH v2 05/13] fuzz/x86_emulate: Implement input_read() and input_avail()

[George Dunlap]

On 10/04/2017 09:22 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> @@ -52,6 +52,22 @@ struct fuzz_state
>>      struct x86_emulate_ops ops;
>>  };
>>  
>> +static inline bool input_available(struct fuzz_state *s, size_t size)
> 
> s can be pointer to const
> 
> Also how about shortening the function name to what the title says?

Sure.

> 
>> +{
>> +    return s->data_index + size < s->data_num;
> 
> Shouldn't this be <= ?

Yes actually.

> 
>> +}
>> +
>> +static inline bool input_read(struct fuzz_state *s, void *dst, size_t size)
>> +{
>> +    if ( !input_available(s, size) )
>> +        return 0;
> 
> false
> 
>> +
>> +    memcpy(dst, &s->corpus->data[s->data_index], size);
>> +    s->data_index += size;
>> +
>> +    return 1;
> 
> true

Ack.

 -George

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

Re: [PATCH v2 06/13] fuzz/x86_emulate: Rename the file containing the wrapper code

[George Dunlap]

On 10/04/2017 09:23 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> --- a/tools/fuzz/x86_instruction_emulator/Makefile
>> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
>> @@ -18,22 +18,22 @@ asm:
>>  
>>  asm/%: asm ;
>>  
>> -x86_emulate.c x86_emulate.h: %:
>> +x86_emulate_user.c x86_emulate_user.h: %:
> 
> How about avoiding the names getting even longer? E.g. using
> x86-emulate.[ch] or x86emul-user.[ch] instead?

My original idea was to make it easy to construct the original filename
from the long filename.  I don't have super-strong opinions (mostly
because I think all the options I've seen are pretty bad), but I still
think that this is the least-bad option.

If you have strong opinions about one of the other ones, let me know and
I'll change it.

>> @@ -42,7 +42,7 @@ all: x86-insn-fuzz-all
>>  
>>  .PHONY: distclean
>>  distclean: clean
>> -	rm -f x86_emulate x86_emulate.c x86_emulate.h asm
>> +	rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm
> 
> If you want to stick to the longer names, would you mind taking the
> opportunity to make this just x86_emulate* ?

What if you put something in that directly called
"x86_emulate_user.c.diff" (or something like that) and then ran "make
clean"?

I tend to think that 'make clean' should only clean things that it is
pretty confident were put there by the build system, and not the user.

 -George

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

Re: [PATCH v2 07/13] fuzz/x86_emulate: Add 'afl-cov' target

[George Dunlap]

On 10/04/2017 09:23 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> --- a/tools/fuzz/README.afl
>> +++ b/tools/fuzz/README.afl
>> @@ -41,3 +41,17 @@ Use the x86 instruction emulator fuzzer as an example.
>>     $ $AFLPATH/afl-fuzz -t 1000 -i testcase_dir -o findings_dir -- ./afl-harness
>>  
>>  Please see AFL documentation for more information.
>> +
>> +# GENERATING COVERAGE INFORMATION
>> +
>> +To use afl-cov or gcov, you need a separate binary instrumented to
>> +generate coverage data.  To do this, use the target `afl-cov`:
>> +
>> +    $ make afl-cov #produces afl-harness-cov
>> +
>> +NOTE: Please also note that the coverage instrumentation hard-codes
>> +the absolute path for the instrumentation read and write files in the
>> +binary; so coverage data will always show up in the build directory no
>> +matter where you run the binary from.
>> +
>> +Please see afl-cov and/or gcov documentation for more information.
>> \ No newline at end of file
> 
> Would you please add the missing newline?

Ack

> 
>> --- a/tools/fuzz/x86_instruction_emulator/Makefile
>> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
>> @@ -23,19 +23,34 @@ x86_emulate_user.c x86_emulate_user.h: %:
>>  
>>  CFLAGS += $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -I.
>>  
>> +GCOV_FLAGS=--coverage
> 
> := ?

Ack

> 
>>  x86.h := asm/x86-vendors.h asm/x86-defns.h asm/msr-index.h
>>  x86_emulate.h := x86_emulate_user.h x86_emulate/x86_emulate.h $(x86.h)
>>  
>> -x86_emulate_user.o: x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
>> +X86_EMULATE_INPUTS = x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
>> +x86_emulate_user.o: $(X86_EMULATE_INPUTS)
>> +
>> +x86_emulate_user-cov.o: $(X86_EMULATE_INPUTS)
>> +	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) -o $@ x86_emulate_user.c
>>  
>>  fuzz-emul.o: $(x86_emulate.h)
>>  
>> +fuzz-emul-cov.o: fuzz-emul.c $(x86_emulate.h)
>> +	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) -o $@ fuzz-emul.c
>> +
>> +afl-harness-cov.o: afl-harness.c
>> +	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) $^ -o $@
> 
> Rather than effectively repeating this command three time, I think
> someone else had already suggested to use a pattern rule instead.

What do you mean "three times"?  There's only  one *-cov.o file which
can possibly be created by a generic rule, and that's this one.   (The
others all have special formulas already.)  Is it really worth making a
generic rule for a single instance?

>> @@ -46,7 +61,7 @@ distclean: clean
>>  
>>  .PHONY: clean
>>  clean:
>> -	rm -f *.a *.o .*.d afl-harness
>> +	rm -f *.a *.o .*.d afl-harness afl-harness-cov *.gcda *.gcno *.gcov
> 
> Perhaps simply *.gc* to cover for possible future generated file types?

If I knew that this wouldn't match files like "foo.gcov-notes.txt" I'd
be fine with it.  I'll change it if you insist but I think it's probably
better the way it is for now.

 -George

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

Re: [PATCH v2 09/13] fuzz/x86_emulate: Move all state into fuzz_state

[George Dunlap]

On 10/04/2017 09:25 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> @@ -39,6 +33,12 @@ struct fuzz_corpus
>>   */
>>  struct fuzz_state
>>  {
>> +    unsigned long options;
>> +    unsigned long cr[5];
>> +    uint64_t msr[MSR_INDEX_MAX];
>> +    struct segment_register segments[SEG_NUM];
>> +    struct cpu_user_regs regs;
>> +
>>      /* Fuzzer's input data. */
>>      struct fuzz_corpus *corpus;
>>  
>> @@ -51,6 +51,8 @@ struct fuzz_state
>>      /* Emulation ops, some of which are disabled based on corpus->options. */
>>      struct x86_emulate_ops ops;
>>  };
>> +#define DATA_OFFSET offsetof(struct fuzz_state, corpus)
>> +
>>  
> 
> Personally I think this would better be placed right between the two
> respective fields in the structure, for it to at the same time serve as
> a clear indication that it needs either changing when a field would be
> inserted there, or the insertion be done elsewhere.

That sounds like a good idea.

> Also please don't
> add another blank line here.

Ack.

>> @@ -760,12 +761,11 @@ static void disable_hooks(struct x86_emulate_ctxt *ctxt)
>>  static void sanitize_input(struct x86_emulate_ctxt *ctxt)
>>  {
>>      struct fuzz_state *s = ctxt->data;
>> -    struct fuzz_corpus *c = s->corpus;
>> -    struct cpu_user_regs *regs = &c->regs;
>> -    unsigned long bitmap = c->options;
>> +    struct cpu_user_regs *regs = ctxt->regs;
> 
> I think this would more obviously look like the equivalent of the old
> code when being &s->regs, but the net effect is the same afaict, so it
> doesn't really matter.
> 
> In any event (with at least the extra blank line removed)
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks,
 -George

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

Re: [PATCH v2 08/13] fuzz/x86_emulate: Take multiple test files for inputs

[George Dunlap]

On 10/04/2017 09:24 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
>> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
>> @@ -16,6 +16,8 @@ int main(int argc, char **argv)
>>  {
>>      size_t size;
>>      FILE *fp = NULL;
>> +    int count = 0;
>> +    int max;
> 
> Generally speaking these should be unsigned int, but I see how this
> collides with the types of the variables max is being calculated from.
> In any event both could go on a single line.

Ack

> 
>> @@ -66,11 +70,14 @@ int main(int argc, char **argv)
>>      __AFL_INIT();
>>  
>>      while ( __AFL_LOOP(1000) )
>> +#else
>> +    for( count = 0; count < max; count++ )
> 
> Initially I've thought the initializer on count was pointless further
> up because of the re-initialization here. Of course that's needed
> because of the #if/#else this sits in. Hence I wonder whether omitting
> the assignment here wouldn't be appropriate - it wouldn't really be
> wromng for a compiler to warn about this redundancy.

Could do that I suppose.  The other option would be to change the other
loop to `for ( count = 0; __AFL_LOOP(1000); )`

Let me know if you have any preference.

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

Thanks,
 -George

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

Re: [PATCH v2 06/13] fuzz/x86_emulate: Rename the file containing the wrapper code

[Jan Beulich]

>>> On 04.10.17 at 18:34, <george.dunlap@citrix.com> wrote:
> On 10/04/2017 09:23 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>> --- a/tools/fuzz/x86_instruction_emulator/Makefile
>>> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
>>> @@ -18,22 +18,22 @@ asm:
>>>  
>>>  asm/%: asm ;
>>>  
>>> -x86_emulate.c x86_emulate.h: %:
>>> +x86_emulate_user.c x86_emulate_user.h: %:
>> 
>> How about avoiding the names getting even longer? E.g. using
>> x86-emulate.[ch] or x86emul-user.[ch] instead?
> 
> My original idea was to make it easy to construct the original filename
> from the long filename.  I don't have super-strong opinions (mostly
> because I think all the options I've seen are pretty bad), but I still
> think that this is the least-bad option.
> 
> If you have strong opinions about one of the other ones, let me know and
> I'll change it.

Well, together with the suggested alternatives being shorter,
they also slightly improve word completion behavior when typing
in partial file names, so yes, I'd really appreciate renaming them
(and I've listed the suggestions above in the order of my
preference).

>>> @@ -42,7 +42,7 @@ all: x86-insn-fuzz-all
>>>  
>>>  .PHONY: distclean
>>>  distclean: clean
>>> -	rm -f x86_emulate x86_emulate.c x86_emulate.h asm
>>> +	rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm
>> 
>> If you want to stick to the longer names, would you mind taking the
>> opportunity to make this just x86_emulate* ?
> 
> What if you put something in that directly called
> "x86_emulate_user.c.diff" (or something like that) and then ran "make
> clean"?
> 
> I tend to think that 'make clean' should only clean things that it is
> pretty confident were put there by the build system, and not the user.

Ah, yes, I see your point, albeit I don't fully agree: I would
actually prefer "make clean" to leave a clean tree, not one
with user created files left in. But indeed that's a matter of
taste.

Jan


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

Re: [PATCH v2 07/13] fuzz/x86_emulate: Add 'afl-cov' target

[Jan Beulich]

>>> On 04.10.17 at 18:48, <george.dunlap@citrix.com> wrote:
> On 10/04/2017 09:23 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>>  x86.h := asm/x86-vendors.h asm/x86-defns.h asm/msr-index.h
>>>  x86_emulate.h := x86_emulate_user.h x86_emulate/x86_emulate.h $(x86.h)
>>>  
>>> -x86_emulate_user.o: x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
>>> +X86_EMULATE_INPUTS = x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
>>> +x86_emulate_user.o: $(X86_EMULATE_INPUTS)
>>> +
>>> +x86_emulate_user-cov.o: $(X86_EMULATE_INPUTS)
>>> +	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) -o $@ x86_emulate_user.c
>>>  
>>>  fuzz-emul.o: $(x86_emulate.h)
>>>  
>>> +fuzz-emul-cov.o: fuzz-emul.c $(x86_emulate.h)
>>> +	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) -o $@ fuzz-emul.c
>>> +
>>> +afl-harness-cov.o: afl-harness.c
>>> +	$(CC) -c $(CFLAGS) $(GCOV_FLAGS) $^ -o $@
>> 
>> Rather than effectively repeating this command three time, I think
>> someone else had already suggested to use a pattern rule instead.
> 
> What do you mean "three times"?  There's only  one *-cov.o file which
> can possibly be created by a generic rule, and that's this one.   (The
> others all have special formulas already.)  Is it really worth making a
> generic rule for a single instance?

All three rules could be changed to use $< afaict, and then they're
all identical.

>>> @@ -46,7 +61,7 @@ distclean: clean
>>>  
>>>  .PHONY: clean
>>>  clean:
>>> -	rm -f *.a *.o .*.d afl-harness
>>> +	rm -f *.a *.o .*.d afl-harness afl-harness-cov *.gcda *.gcno *.gcov
>> 
>> Perhaps simply *.gc* to cover for possible future generated file types?
> 
> If I knew that this wouldn't match files like "foo.gcov-notes.txt" I'd
> be fine with it.  I'll change it if you insist but I think it's probably
> better the way it is for now.

Okay, same matter of taste as in the earlier patch. I.e. no, I
won't insist.

Jan


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

Re: [PATCH v2 08/13] fuzz/x86_emulate: Take multiple test files for inputs

[Jan Beulich]

>>> On 04.10.17 at 18:58, <george.dunlap@citrix.com> wrote:
> On 10/04/2017 09:24 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>> @@ -66,11 +70,14 @@ int main(int argc, char **argv)
>>>      __AFL_INIT();
>>>  
>>>      while ( __AFL_LOOP(1000) )
>>> +#else
>>> +    for( count = 0; count < max; count++ )
>> 
>> Initially I've thought the initializer on count was pointless further
>> up because of the re-initialization here. Of course that's needed
>> because of the #if/#else this sits in. Hence I wonder whether omitting
>> the assignment here wouldn't be appropriate - it wouldn't really be
>> wromng for a compiler to warn about this redundancy.
> 
> Could do that I suppose.  The other option would be to change the other
> loop to `for ( count = 0; __AFL_LOOP(1000); )`
> 
> Let me know if you have any preference.

No preference, your alternative is fine.

Jan


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

Re: [PATCH v2 06/13] fuzz/x86_emulate: Rename the file containing the wrapper code

[George Dunlap]

On 10/05/2017 10:01 AM, Jan Beulich wrote:
>>>> On 04.10.17 at 18:34, <george.dunlap@citrix.com> wrote:
>> On 10/04/2017 09:23 AM, Jan Beulich wrote:
>>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>>> --- a/tools/fuzz/x86_instruction_emulator/Makefile
>>>> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
>>>> @@ -18,22 +18,22 @@ asm:
>>>>  
>>>>  asm/%: asm ;
>>>>  
>>>> -x86_emulate.c x86_emulate.h: %:
>>>> +x86_emulate_user.c x86_emulate_user.h: %:
>>>
>>> How about avoiding the names getting even longer? E.g. using
>>> x86-emulate.[ch] or x86emul-user.[ch] instead?
>>
>> My original idea was to make it easy to construct the original filename
>> from the long filename.  I don't have super-strong opinions (mostly
>> because I think all the options I've seen are pretty bad), but I still
>> think that this is the least-bad option.
>>
>> If you have strong opinions about one of the other ones, let me know and
>> I'll change it.
> 
> Well, together with the suggested alternatives being shorter,
> they also slightly improve word completion behavior when typing
> in partial file names, so yes, I'd really appreciate renaming them
> (and I've listed the suggestions above in the order of my
> preference).

Ok.

> 
>>>> @@ -42,7 +42,7 @@ all: x86-insn-fuzz-all
>>>>  
>>>>  .PHONY: distclean
>>>>  distclean: clean
>>>> -	rm -f x86_emulate x86_emulate.c x86_emulate.h asm
>>>> +	rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm
>>>
>>> If you want to stick to the longer names, would you mind taking the
>>> opportunity to make this just x86_emulate* ?
>>
>> What if you put something in that directly called
>> "x86_emulate_user.c.diff" (or something like that) and then ran "make
>> clean"?
>>
>> I tend to think that 'make clean' should only clean things that it is
>> pretty confident were put there by the build system, and not the user.
> 
> Ah, yes, I see your point, albeit I don't fully agree: I would
> actually prefer "make clean" to leave a clean tree, not one
> with user created files left in. But indeed that's a matter of
> taste.

Well if that's the case we should have a whitelist, and do something
like "ls -a | (filter whitelist) | xargs rm -f".  But I think `git clean
-ffdx` does that job for most people these days.

 -George

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

Re: [PATCH v2 10/13] fuzz/x86_emulate: Make input more compact

[George Dunlap]

On 10/04/2017 09:26 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> @@ -22,13 +25,17 @@ int main(int argc, char **argv)
>>      setbuf(stdin, NULL);
>>      setbuf(stdout, NULL);
>>  
>> +    opt_compact = true;
> 
> How about giving the variable an initializer instead?

Actually, if we want fuzz-emul.c to be usable by itself (e.g., for the
Google ossfuz project), we *must* use a static initializer from within
fuzz-emul.c for it to have the correct defaults.  I'll change that...

> 
>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> @@ -53,6 +53,15 @@ struct fuzz_state
>>  };
>>  #define DATA_OFFSET offsetof(struct fuzz_state, corpus)
>>  
>> +bool opt_compact;
>> +
>> +unsigned int fuzz_minimal_input_size(void)
>> +{
>> +    if ( opt_compact )
>> +        return sizeof(unsigned long) + 1;
> 
> What is this value choice based on / derived from? Oh, judging from
> code further down it may be one more than the size of the options
> field, in which case it should be sizeof(...->options) here.

What about renaming DATA_OFFSET to DATA_SIZE_FULL, and adding
DATA_SIZE_COMPACT?

Then is could be:

    return (opt_compact ? DATA_SIZE_COMPACT : DATA_SIZE_FULL) + 1;

>> @@ -647,9 +656,81 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
>>  {
>>      struct fuzz_state *s = ctxt->data;
>>  
>> -    /* Fuzz all of the state in one go */
>> -    if (!input_read(s, s, DATA_OFFSET))
>> -        exit(-1);
>> +    if ( !opt_compact )
>> +    {
>> +        /* Fuzz all of the state in one go */
>> +        if (!input_read(s, s, DATA_OFFSET))
> 
> Missing blanks.

Ack

> 
>> +            exit(-1);
>> +        return;
>> +    }
>> +
>> +    /* Modify only select bits of state */
>> +
>> +    /* Always read 'options' */
>> +    if ( !input_read(s, &s->options, sizeof(s->options)) )
>> +        return;
>> +    
>> +    while(1) {
> 
> Style. And for compatibility (read: no warnings) with as wide a range
> of compilers as possible, generally for ( ; ; ) is better to use.

I can do that; but would you mind explaining?  What kinds of compilers
don't like while(1)?

>> +        uint16_t offset;
>> +
>> +        /* Read 16 bits to decide what bit of state to modify */
>> +        if ( !input_read(s, &offset, sizeof(offset)) )
>> +            return;
> 
> Doesn't this suggest minimal input size wants to be one higher than
> what you currently enforce? And isn't the use of uint16_t here in
> conflict with the description talking about reading a byte every time?

Hmm, actually it rather implies that it should be one less... with the
new format there's no way to guarantee that the very first insn_fetch
will have any data to read.

>> +        /* 
>> +         * Then decide if it's "pointing to" different bits of the
>> +         * state 
>> +         */
>> +
>> +        /* cr[]? */
>> +        if ( offset < 5 )
> 
> ARRAY_SIZE()

Ack

>> +        {
>> +            if ( !input_read(s, s->cr + offset, sizeof(*s->cr)) )
>> +                return;
>> +            printf("Setting CR %d to %lx\n", offset, s->cr[offset]);
>> +            continue;
>> +        }
>> +        
>> +        offset -= 5;
> 
> Same here then.
> 
>> +        /* msr[]? */
>> +        if ( offset < MSR_INDEX_MAX )
> 
> Even here (and below) use of ARRAY_SIZE() may be better.
> 
>> +        {
>> +            if ( !input_read(s, s->msr + offset, sizeof(*s->msr)) )
>> +                return;
>> +            printf("Setting MSR i%d (%x) to %lx\n", offset,
>> +                   msr_index[offset], s->msr[offset]);
>> +            continue;
>> +        }
>> +
>> +        offset -= MSR_INDEX_MAX;
>> +
>> +        /* segments[]? */
>> +        if ( offset < SEG_NUM )
>> +        {
>> +            if ( !input_read(s, s->segments + offset, sizeof(*s->segments)) )
>> +                return;
>> +            printf("Setting Segment %d\n", offset);
>> +            continue;
>> +            
>> +        }
>> +
>> +        offset -= SEG_NUM;
>> +
>> +        /* regs? */
>> +        if ( offset < sizeof(struct cpu_user_regs)
>> +             && offset + sizeof(uint64_t) <= sizeof(struct cpu_user_regs) )
>> +        {
>> +            if ( !input_read(s, ((char *)ctxt->regs) + offset, sizeof(uint64_t)) )
>> +                return;
>> +            printf("Setting cpu_user_regs offset %x\n", offset);
>> +            continue;
>> +        }
>> +
>> +        /* None of the above -- take that as "start emulating" */
>> +        
>> +        return;
>> +    }
> 
> Having come here I wonder whether the use of "byte" in the description
> is right, and you mean "uint8_t offset" above, as you're far from
> consuming the entire 256 value range.

Isn't cpu_user_regs larger than 256 bytes?  And in any case, the offset
will become larger than 256 bytes one we include the FPU state.

> Additionally, was the order of elements here chosen for any specific
> reason? It would seem to me that elements having a more significant
> effect on emulation may be worth filling first, and I'm not convinced
> the "all CRs, all MSRs, all SREGs, all GPRs" order matches that.

I'm not aware of any particular order; it's probably some combination of
"the order they were in the cpu_regs struct" and "the order in which I
found it useful to add them".  Given that the input will be more or less
random, I don't think the order in the struct will have too much of an
impact on the order in which AFL explores them.

If you have an alternative suggestion for an order you think would be
more logical I'm happy to rearrange the structure.

 -George

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

Re: [PATCH v2 10/13] fuzz/x86_emulate: Make input more compact

[Jan Beulich]

>>> On 05.10.17 at 17:04, <george.dunlap@citrix.com> wrote:
> On 10/04/2017 09:26 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>>> @@ -53,6 +53,15 @@ struct fuzz_state
>>>  };
>>>  #define DATA_OFFSET offsetof(struct fuzz_state, corpus)
>>>  
>>> +bool opt_compact;
>>> +
>>> +unsigned int fuzz_minimal_input_size(void)
>>> +{
>>> +    if ( opt_compact )
>>> +        return sizeof(unsigned long) + 1;
>> 
>> What is this value choice based on / derived from? Oh, judging from
>> code further down it may be one more than the size of the options
>> field, in which case it should be sizeof(...->options) here.
> 
> What about renaming DATA_OFFSET to DATA_SIZE_FULL, and adding
> DATA_SIZE_COMPACT?
> 
> Then is could be:
> 
>     return (opt_compact ? DATA_SIZE_COMPACT : DATA_SIZE_FULL) + 1;

Looks fine.

>>> +            exit(-1);
>>> +        return;
>>> +    }
>>> +
>>> +    /* Modify only select bits of state */
>>> +
>>> +    /* Always read 'options' */
>>> +    if ( !input_read(s, &s->options, sizeof(s->options)) )
>>> +        return;
>>> +    
>>> +    while(1) {
>> 
>> Style. And for compatibility (read: no warnings) with as wide a range
>> of compilers as possible, generally for ( ; ; ) is better to use.
> 
> I can do that; but would you mind explaining?  What kinds of compilers
> don't like while(1)?

In various projects of my own I have on (and targeting) Windows
I see this with almost every compiler I happen to use there. Hence
I've started to avoid the construct many years ago.

>>> +        {
>>> +            if ( !input_read(s, s->msr + offset, sizeof(*s->msr)) )
>>> +                return;
>>> +            printf("Setting MSR i%d (%x) to %lx\n", offset,
>>> +                   msr_index[offset], s->msr[offset]);
>>> +            continue;
>>> +        }
>>> +
>>> +        offset -= MSR_INDEX_MAX;
>>> +
>>> +        /* segments[]? */
>>> +        if ( offset < SEG_NUM )
>>> +        {
>>> +            if ( !input_read(s, s->segments + offset, sizeof(*s->segments)) )
>>> +                return;
>>> +            printf("Setting Segment %d\n", offset);
>>> +            continue;
>>> +            
>>> +        }
>>> +
>>> +        offset -= SEG_NUM;
>>> +
>>> +        /* regs? */
>>> +        if ( offset < sizeof(struct cpu_user_regs)
>>> +             && offset + sizeof(uint64_t) <= sizeof(struct cpu_user_regs) )
>>> +        {
>>> +            if ( !input_read(s, ((char *)ctxt->regs) + offset, sizeof(uint64_t)) )
>>> +                return;
>>> +            printf("Setting cpu_user_regs offset %x\n", offset);
>>> +            continue;
>>> +        }
>>> +
>>> +        /* None of the above -- take that as "start emulating" */
>>> +        
>>> +        return;
>>> +    }
>> 
>> Having come here I wonder whether the use of "byte" in the description
>> is right, and you mean "uint8_t offset" above, as you're far from
>> consuming the entire 256 value range.
> 
> Isn't cpu_user_regs larger than 256 bytes?  And in any case, the offset
> will become larger than 256 bytes one we include the FPU state.

Oh, of course. I've somehow stopped summing at the point 
SEG_NUM is being subtracted.

>> Additionally, was the order of elements here chosen for any specific
>> reason? It would seem to me that elements having a more significant
>> effect on emulation may be worth filling first, and I'm not convinced
>> the "all CRs, all MSRs, all SREGs, all GPRs" order matches that.
> 
> I'm not aware of any particular order; it's probably some combination of
> "the order they were in the cpu_regs struct" and "the order in which I
> found it useful to add them".  Given that the input will be more or less
> random, I don't think the order in the struct will have too much of an
> impact on the order in which AFL explores them.

Well, yes, except for very small input (which will leave "higher"
parts unrandomized).

> If you have an alternative suggestion for an order you think would be
> more logical I'm happy to rearrange the structure.

Generally I'd expect GPRs to be most relevant to change value,
but them going first might be sort of ugly, as they're quite big.
For the others I'd say SREGs, CRs, then MSRs. But if it doesn't
really matter (i.e. if small input isn't of much concern, as you
suggest above), you may as well leave things the way they are.

Jan

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

Re: [PATCH v2 11/13] fuzz/x86_emulate: Add --rerun option to try to track down instability

[George Dunlap]

On 10/04/2017 09:27 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> --- a/tools/fuzz/x86_instruction_emulator/afl-harness.c
>> +++ b/tools/fuzz/x86_instruction_emulator/afl-harness.c
>> @@ -14,6 +14,7 @@ extern unsigned int fuzz_minimal_input_size(void);
>>  static uint8_t input[INPUT_SIZE];
>>  
>>  extern bool opt_compact;
>> +extern bool opt_rerun;
> 
> Seeing a second such variable appear, I think it would really be better
> to introduce a local header, included by both producer and consumer.

Yes, sounds good.

> 
>> @@ -886,21 +896,138 @@ int LLVMFuzzerInitialize(int *argc, char ***argv)
>>      return 0;
>>  }
>>  
>> -int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>> +void setup_fuzz_state(struct fuzz_state *state, const uint8_t *data_p, size_t size)
> 
> static (also for other new helper functions)?

Ack

> 
>>  {
>> -    struct fuzz_state state = {
>> -        .ops = all_fuzzer_ops,
>> -    };
>> -    struct x86_emulate_ctxt ctxt = {
>> -        .data = &state,
>> -        .regs = &state.regs,
>> -        .addr_size = 8 * sizeof(void *),
>> -        .sp_size = 8 * sizeof(void *),
>> -    };
>> +    memset(state, 0, sizeof(*state));
>> +    state->corpus = (struct fuzz_corpus *)data_p;
> 
> Please don't cast away constness here. Perhaps best to make the parameter
> const void *, allowing for the cast to be dropped altogether.

Didn't notice that I was casting away const-ness, because state->corpus
is const. :-)  But I like `const void *`.

> 
>> +int runtest(struct fuzz_state *state) {
>>      int rc;
>>  
>> -    /* Reset all global state variables */
>> -    memset(&input, 0, sizeof(input));
>> +    struct x86_emulate_ctxt *ctxt = &state->ctxt;
>> +    
>> +    state->ops = all_fuzzer_ops;
>> +
>> +    ctxt->data = state;
>> +    ctxt->regs = &state->regs;
>> +    ctxt->addr_size = ctxt->sp_size = 8 * sizeof(void *);
> 
> Is this actually necessary? I don't see a way for set_sizes() to be
> bypassed.

I was just duplicating the functionality that was already there (these
were initialized at declaration).  My instinct is to want to leave these
initialize these for safety, but the code definitely should call
set_sizes() first, so I'll take this out.

> 
>> +void compare_states(struct fuzz_state state[2])
>> +{
>> +    // First zero any "internal" pointers
>> +    state[0].corpus = state[1].corpus = NULL;
>> +    state[0].ctxt.data = state[1].ctxt.data = NULL;
>> +    state[0].ctxt.regs = state[1].ctxt.regs = NULL;
>> +
>> +    
> 
> No double blank lines please.
> 
>> +    if ( memcmp(&state[0], &state[1], sizeof(struct fuzz_state)) )
>> +    {
>> +        int i;
> 
> unsigned int (and then %u in the format strings below)

Is there really an advantage to specifying 'unsigned int' for something
like a loop variable?  It hardly seems worth the effort to consider
signed / unsigned in such a case.

>> +        printf("State mismatch\n");
>> +
>> +        for ( i=0; i<5; i++)
> 
> Blanks missing and please use ARRAY_SIZE() again (also further down).

Ack.

> 
>> +            if ( state[0].cr[i] != state[1].cr[i] )
>> +                printf("cr[%d]: %lx != %lx\n",
>> +                       i, state[0].cr[i], state[1].cr[i]);
>> +        
>> +        for ( i=0; i<MSR_INDEX_MAX; i++)
>> +            if ( state[0].msr[i] != state[1].msr[i] )
>> +                printf("msr[%d]: %lx != %lx\n",
>> +                       i, state[0].msr[i], state[1].msr[i]);
>> +        
>> +        for ( i=0; i<SEG_NUM; i++)
>> +            if ( memcmp(&state[0].segments[i], &state[1].segments[i],
>> +                        sizeof(state[0].segments[0])) )
>> +                printf("segments[%d] differ!\n", i);
> 
> The actual values would likely be helpful to be printed here, just like
> you do for all other state elements.

Sure.

> 
>> +        if ( state[0].data_num != state[1].data_num )
>> +            printf("data_num: %lx !=  %lx\n", state[0].data_num,
>> +                   state[1].data_num);
>> +        if ( state[0].data_index != state[1].data_index )
>> +            printf("data_index: %lx !=  %lx\n", state[0].data_index,
>> +                   state[1].data_index);
>> +
>> +        if ( memcmp(&state[0].regs, &state[1].regs, sizeof(state[0].regs)) )
>> +        {
>> +            printf("registers differ!\n");
>> +            /* Print If Not Equal */
>> +#define PINE(elem)\
>> +            if ( state[0].elem != state[1].elem ) \
>> +                printf(#elem " differ: %lx != %lx\n", \
>> +                       (unsigned long)state[0].elem, \
>> +                       (unsigned long)state[1].elem)
>> +            PINE(regs.r15);
>> +            PINE(regs.r14);
>> +            PINE(regs.r13);
>> +            PINE(regs.r12);
>> +            PINE(regs.rbp);
>> +            PINE(regs.rbx);
>> +            PINE(regs.r10);
>> +            PINE(regs.r11);
>> +            PINE(regs.r9);
>> +            PINE(regs.r8);
>> +            PINE(regs.rax);
>> +            PINE(regs.rcx);
>> +            PINE(regs.rdx);
>> +            PINE(regs.rsi);
>> +            PINE(regs.rdi);
>> +
>> +            for ( i = offsetof(struct cpu_user_regs, error_code) / sizeof(unsigned);
>> +                  i < sizeof(state[1].regs)/sizeof(unsigned); i++ )
>> +            {
>> +                printf("[%04lu] %08x %08x\n",
> 
> I think this wants to be %04zu (or perhaps better %4zu or %04zx). Same
> for ctxt printing further down.
> 
>> @@ -908,7 +1035,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>>          return 1;
>>      }
>>  
>> -    if ( size > sizeof(input) )
>> +    if ( size > sizeof(struct fuzz_corpus) )
> 
> What's the difference between the two variants?

One fewer 'dereferences'.  Rather than input -> struct fuzz_corpus ->
[structure definition], you can just do struct fuzz_corpus -> [structure
definition].

>> @@ -916,25 +1043,24 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>>  
>>      memcpy(&input, data_p, size);
>>  
>> -    state.corpus = &input;
>> -    state.data_num = size;
>> -
>> -    setup_state(&ctxt);
>> +    setup_fuzz_state(&state[0], data_p, size);
>> +    
>> +    if ( opt_rerun )
>> +        printf("||| INITIAL RUN |||\n");
>> +    
>> +    runtest(&state[0]);
>>  
>> -    sanitize_input(&ctxt);
>> +    if ( !opt_rerun )
>> +        return 0;
> 
> Could I talk you into inverting the condition such that there'll be
> only a single "return 0" at the end of the function?

Why is that valuable?

If I don't return here, then the rerun code has to be indented, which to
me makes it slightly more difficult to see that it's identical to the
state setup & running code above.

> And then - has this patch actually helped pinpoint any problems? The
> ones deaslt with by the next patch perhaps?

Yes, it helped find the one dealt with in the subsequent patch.
Surprisingly, it didn't find anything else.

Since the patch represented a non-trivial amount of work, I thought it
might be useful to include so nobody would have to re-implement it again
in the future.  But I'd also be happy discarding this patch, as it's
fairly invasive and I don't expect it to be used very often.

Let me know what you think so I know whether to try to print something
sensible for the segment differences or not bother. :-)

 -George

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

Re: [PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state

[George Dunlap]

On 10/04/2017 09:28 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>>  };
>>  #undef SET
>>  
>> +static void _set_fpu_state(char *fxsave, bool store)
>> +{
>> +    if ( cpu_has_fxsr )
>> +    {
>> +        static union __attribute__((__aligned__(16))) {
>> +            char x[464];
> 
> The final part of the save area isn't being written, yes, but is it
> really worth saving the few bytes of stack space here, rather than
> having the expected 512 as array dimension?

So I didn't actually look into this very much; I mainly just hacked at
it until it seemed to work.  I copied-and-pasted emul_test_init() from
x86_emulate.c (which is where the 464 came from), then copied some
scraps of asm from stackoverflow.

>> +            struct {
>> +                uint32_t other[6];
>> +                uint32_t mxcsr;
>> +                uint32_t mxcsr_mask;
>> +                /* ... */
>> +            };
>> +        } *fxs;
>> +
>> +        fxs = (typeof(fxs)) fxsave;
>> +
>> +        if ( store ) {
> 
> Style.
> 
>> +            char null[512] __attribute__((aligned(16))) = { 0 };
> 
> No need for the 0 and a blank line between declaration and statements
> please.
> 
>> +            asm volatile(" fxrstor %0; "::"m"(*null));
>> +            asm volatile(" fxrstor %0; "::"m"(*fxsave));
> 
> Style again - these want to follow the
> 
>     asm volatile ( "..." :: "m" (...) )
> 
> form. No need for the ; following the instructions.
>
>> +        }
>> +        
>> +        asm volatile( "fxsave %0" : "=m" (*fxs) );
> 
> This is pretty confusing, the more with the different variable names
> used which point to the same piece of memory. You basically store back
> into the area you've read from. Is the caller expecting the memory area
> to change? Is this being done other than for convenience to not have
> another instance of scratch space on the stack? Some comment on the
> intentions may be helpful here.

Yes, sorry for the different variable names.  I should have done a
better clean-up of this patch.

As for why it's doing an fxsave after just doing an fxrstor: I had the
idea that what came out via fxsave might not be the same as what was
written via fxrstor (i.e., the instruction would "interpret" the data),
particularly as what went in would be completely random fuzzed state.
The idea behind doing the restore / save was to "sanitize" the state in
the state struct to look more like real input data.

> The function's parameter name being "store" adds to the confusion,
> since what it controls is actually what we call "load" on x86 (or
> "restore" following the insn mnemonics).

I chose 'store' as the argument name before I realized that fxrstor was
"fx restore" and not "fxr store".

Do you think 'write' would be suitable?  Names like "restore" or "load"
make sense if you're thinking about things from the processor's
perspective (as the architects certainly were); but they make less sense
from a programmer's perspective, since (to me anyway) it seems like I'm
writing to or reading from the FPU unit (rather than loading/restoring
or saving).

If you don't like 'write' I'll change it to 'restore'.

> And then - what about YMM register state? Other more exotic registers
> (like the BND* ones) may indeed not be that relevant to fuzz yet.

I can look into that if you want, or if you want to give me some runes
to copy in I'm happy to do that as well.

>> @@ -737,6 +780,17 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
>>              printf("Setting cpu_user_regs offset %x\n", offset);
>>              continue;
>>          }
>> +        offset -= sizeof(struct cpu_user_regs);
>> +
>> +        /* Fuzz fxsave state */
>> +        if ( offset < 128 )
>> +        {
>> +            if ( !input_read(s, s->fxsave + (offset * 4), 4) )
>> +                return;
>> +            printf("Setting fxsave offset %x\n", offset * 4);
> 
> What's this 32-bit granularity derived from?

Just seemed like a good-sized chunk.  Doing it byte-by-byte seemed to be
"wasting" input on offsets (as in the input you'd have a 2-byte 'offset'
followed by a one-byte bit of data).  This way you have a 2-byte offset
and a 4-byte chunk of data that you write.

Let me know if you think there's a better size for chunks of data to
write.  In any case I'll add a comment in here to let people know that
the size is arbitrary.

>> @@ -883,6 +937,9 @@ static void sanitize_state(struct x86_emulate_ctxt *ctxt)
>>          s->segments[x86_seg_cs].db = 0;
>>          s->segments[x86_seg_ss].db = 0;
>>      }
>> +
>> +    /* Setting this value seems to cause crashes in fxrstor */
>> +    *((unsigned int *)(s->fxsave) + 6) = 0;
> 
> That's the MXCSR field - instead of storing zero you want to mask with
> mxcsr_mask. To avoid the ugly literal 6 (and to make clear what it is
> that needs adjustment here) it may also be worthwhile widening the
> scope of the type declared in _set_fpu_state() and use it for struct
> fuzz_state's fxsave field.

Got it.

 -George

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

Re: [PATCH v2 11/13] fuzz/x86_emulate: Add --rerun option to try to track down instability

[Jan Beulich]

>>> George Dunlap <george.dunlap@citrix.com> 10/05/17 6:13 PM >>>
>On 10/04/2017 09:27 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>> +    if ( memcmp(&state[0], &state[1], sizeof(struct fuzz_state)) )
>>> +    {
>>> +        int i;
>> 
>> unsigned int (and then %u in the format strings below)
>
>Is there really an advantage to specifying 'unsigned int' for something
>like a loop variable?  It hardly seems worth the effort to consider
>signed / unsigned in such a case.

The latest when a loop variable is being used as array index it does matter on
most 64-bit architectures: Zero-extension (to machine word size) is often implied
by other operations, while sign-extension frequently requires an extra insn. This
may not matter much here, but I think it's better to follow the same common
pattern everywhere.

>>> @@ -908,7 +1035,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>>>          return 1;
>>>      }
>>>  
>>> -    if ( size > sizeof(input) )
>>> +    if ( size > sizeof(struct fuzz_corpus) )
>> 
>> What's the difference between the two variants?
>
>One fewer 'dereferences'.  Rather than input -> struct fuzz_corpus ->
>[structure definition], you can just do struct fuzz_corpus -> [structure
>definition].

;-)

>>> @@ -916,25 +1043,24 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>>>  
>>>      memcpy(&input, data_p, size);
>>>  
>>> -    state.corpus = &input;
>>> -    state.data_num = size;
>>> -
>>> -    setup_state(&ctxt);
>>> +    setup_fuzz_state(&state[0], data_p, size);
>>> +    
>>> +    if ( opt_rerun )
>>> +        printf("||| INITIAL RUN |||\n");
>>> +    
>>> +    runtest(&state[0]);
>>>  
>>> -    sanitize_input(&ctxt);
>>> +    if ( !opt_rerun )
>>> +        return 0;
>> 
>> Could I talk you into inverting the condition such that there'll be
>> only a single "return 0" at the end of the function?
>
>Why is that valuable?
>
>If I don't return here, then the rerun code has to be indented, which to
>me makes it slightly more difficult to see that it's identical to the
>state setup & running code above.

Then leave it this way, as being a matter of taste. I generally think that it is
helpful for functions to only have a single "main" return point (i.e. not
counting error paths), for readers to easily see the normal flow.

>> And then - has this patch actually helped pinpoint any problems? The
>> ones deaslt with by the next patch perhaps?
>
>Yes, it helped find the one dealt with in the subsequent patch.
>Surprisingly, it didn't find anything else.
>
>Since the patch represented a non-trivial amount of work, I thought it
>might be useful to include so nobody would have to re-implement it again
>in the future.  But I'd also be happy discarding this patch, as it's
>fairly invasive and I don't expect it to be used very often.

Oh, I'm certainly in favor of keeping this patch. I was rather trying to
understand whether with it in use the main (or all?) source(s) of instability
were found (and taken care of).

Jan


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

Re: [PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state

[Jan Beulich]

>>> George Dunlap <george.dunlap@citrix.com> 10/05/17 7:08 PM >>>
>On 10/04/2017 09:28 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>>>  };
>>>  #undef SET
>>>  
>>> +static void _set_fpu_state(char *fxsave, bool store)
>>> +{
>>> +    if ( cpu_has_fxsr )
>>> +    {
>>> +        static union __attribute__((__aligned__(16))) {
>>> +            char x[464];
>> 
>> The final part of the save area isn't being written, yes, but is it
>> really worth saving the few bytes of stack space here, rather than
>> having the expected 512 as array dimension?
>
>So I didn't actually look into this very much; I mainly just hacked at
>it until it seemed to work.  I copied-and-pasted emul_test_init() from
>x86_emulate.c (which is where the 464 came from), then copied some
>scraps of asm from stackoverflow.

Oh, so it looks like I'm guilty here, as I think it was me who wrote it that
way there. I have to admit I don't really see why I wanted to save on stack
consumption there. In any event I'm then fine for you to leave it that way,
so the two places remain in sync (but I would also be fine if you changed
it here, and I'd then try to remember to clean it up on the other side).

>>> +        }
>>> +        
>>> +        asm volatile( "fxsave %0" : "=m" (*fxs) );
>> 
>> This is pretty confusing, the more with the different variable names
>> used which point to the same piece of memory. You basically store back
>> into the area you've read from. Is the caller expecting the memory area
>> to change? Is this being done other than for convenience to not have
>> another instance of scratch space on the stack? Some comment on the
>> intentions may be helpful here.
>
>Yes, sorry for the different variable names.  I should have done a
>better clean-up of this patch.
>
>As for why it's doing an fxsave after just doing an fxrstor: I had the
>idea that what came out via fxsave might not be the same as what was
>written via fxrstor (i.e., the instruction would "interpret" the data),
>particularly as what went in would be completely random fuzzed state.
>The idea behind doing the restore / save was to "sanitize" the state in
>the state struct to look more like real input data.

Okay, that's what I had guessed. As said, please put this in a comment, the
more that you've realized this doesn't work all by itself (due to the MXCSR field
causing #GP when not sanitized _before_ doing the fxrstor). And the restore
from null then is to pre-init any (theoretical) fields the subsequent restore may
not touch at all?

>> The function's parameter name being "store" adds to the confusion,
>> since what it controls is actually what we call "load" on x86 (or
>> "restore" following the insn mnemonics).
>
>I chose 'store' as the argument name before I realized that fxrstor was
>"fx restore" and not "fxr store".
>
>Do you think 'write' would be suitable?  Names like "restore" or "load"
>make sense if you're thinking about things from the processor's
>perspective (as the architects certainly were); but they make less sense
>from a programmer's perspective, since (to me anyway) it seems like I'm
>writing to or reading from the FPU unit (rather than loading/restoring
>or saving).
>
>If you don't like 'write' I'll change it to 'restore'.

"write" is fine, I think, as would be "ro" or "readonly".

>> And then - what about YMM register state? Other more exotic registers
>> (like the BND* ones) may indeed not be that relevant to fuzz yet.
>
>I can look into that if you want, or if you want to give me some runes
>to copy in I'm happy to do that as well.

As that's not as simple as FXSAVE/FXRSTOR (due to first needing to
discover area sizes) it's perhaps best to simply leave a TODO comment for
now.

>>> @@ -737,6 +780,17 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
>>>              printf("Setting cpu_user_regs offset %x\n", offset);
>>>              continue;
>>>          }
>>> +        offset -= sizeof(struct cpu_user_regs);
>>> +
>>> +        /* Fuzz fxsave state */
>>> +        if ( offset < 128 )
>>> +        {
>>> +            if ( !input_read(s, s->fxsave + (offset * 4), 4) )
>>> +                return;
>>> +            printf("Setting fxsave offset %x\n", offset * 4);
>> 
>> What's this 32-bit granularity derived from?
>
>Just seemed like a good-sized chunk.  Doing it byte-by-byte seemed to be
>"wasting" input on offsets (as in the input you'd have a 2-byte 'offset'
>followed by a one-byte bit of data).  This way you have a 2-byte offset
>and a 4-byte chunk of data that you write.

Well, ideally individual pieces would be taken all-or-nothing, but due to the
varying sizes this would be rather cumbersome. So with the comment about
this being arbitrary add, I think this will be fine for the time being.

Jan


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

Re: [PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state

[Jan Beulich]

>>> On 05.10.17 at 19:08, <george.dunlap@citrix.com> wrote:
> On 10/04/2017 09:28 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>>>  };
>>>  #undef SET
>>>  
>>> +static void _set_fpu_state(char *fxsave, bool store)
>>> +{
>>> +    if ( cpu_has_fxsr )
>>> +    {
>>> +        static union __attribute__((__aligned__(16))) {
>>> +            char x[464];
>> 
>> The final part of the save area isn't being written, yes, but is it
>> really worth saving the few bytes of stack space here, rather than
>> having the expected 512 as array dimension?
> 
> So I didn't actually look into this very much; I mainly just hacked at
> it until it seemed to work.  I copied-and-pasted emul_test_init() from
> x86_emulate.c (which is where the 464 came from), then copied some
> scraps of asm from stackoverflow.

One thing that came to mind in this context: It would perhaps be
useful to not waste input bytes on the unused portions of the
save area. Along those lines it may also be worth considering not
to waste input on the high halves of 64-bit registers as well as
the high 8 GPRs when emulating 32- or 16-bit mode.

Jan


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

Re: [PATCH v2 13/13] fuzz/x86_emulate: Add an option to limit the number of instructions executed

[George Dunlap]

On 10/04/2017 09:28 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> AFL considers a testcase to be a useful addition not only if there are
>> tuples exercised by that testcase which were not exercised otherwise,
>> but also if the *number* of times an individual tuple is exercised
>> changes significantly; in particular, if the number of the highes bit
>> changes (i.e., if it is run 1, 2-3, 4-7, 8-15, &c).
> 
> Perhaps I simply don't know about AFL (yet) to understand how "highest
> bit" matters here, or even whose highest bits there's talk of.

Probably the easiest way to get this would be to read the
'technical_details.txt' [1] document about AFL, specifically the section
"Detecting new behaviors".  The section isn't long, and I'm not sure I
could explain the situation more concisely than the author has there.

[1] http://lcamtuf.coredump.cx/afl/technical_details.txt


>> Unfortunately, one simple way to increase these stats it to execute
>> the same (or similar) instructions multiple times.
> 
> But the change here doesn't look at instruction similarity at all.

I'm talking about how blind changes AFL makes to the input affect what
AFL sees at the "output".

Suppose it has a testcase where instruction A is executed once, and it
sees tuple N executed twice.  Now suppose it morphs the instruction so
instruction A is executed twice.  It will now see tuple N executed 4
times.  This is seen as 'new behavior', and so it will add that as a
'new' test case to its set of interesting things to fuzz.  Then suppose
it morphs one of those so that instruction A is executed four times.
Tuple N will be executed  8 times, which again is new behavior.  The
highest tuple count it sees as unique is 128; so in our example, it will
generate sample inputs up to 64 instructions -- even if the actual path
through the code for each instruction is identical to the
single-instruction one.

A 64-instruction test case will take at least 64x as long to execute as
a 1-instruction test case; and it will generally also take 64x as long
to fuzz.  This makes AFL is spending nearly 1000x as much time fuzzing
that test case as the 1-instruction test case, but for no very good
reason -- if you can't get actual new behavior we care about out of 2-3
instructions, you're not going to get it out of 60 instructions.

IOW, arbitrary numbers of instructions fool AFL into thinking it's found
something new and interesting when it hasn't.  Limiting the number of
instructions should in theory keep AFL from getting distracted with test
cases it thinks are new and unique but aren't.  And we see that for the
old format, this is true.

I suspect there's some number of instructions past which we get
diminishing returns even for the 'compact' format, but since testing
involves running things for 24 hours, there's also a diminishing returns
for that kind of optimization. :-)

>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> @@ -960,10 +960,13 @@ void setup_fuzz_state(struct fuzz_state *state, const uint8_t *data_p, size_t si
>>      state->data_num = size;
>>  }
>>  
>> +int opt_instruction_limit = 0;
> 
> unsigned int (and formally no need for an initializer)
> 
>>  int runtest(struct fuzz_state *state) {
>>      int rc;
>>  
>>      struct x86_emulate_ctxt *ctxt = &state->ctxt;
>> +    int icount = 0;
> 
> unsigned int

Ack

> 
>> @@ -988,7 +991,9 @@ int runtest(struct fuzz_state *state) {
>>  
>>          rc = x86_emulate(ctxt, &state->ops);
>>          printf("Emulation result: %d\n", rc);
>> -    } while ( rc == X86EMUL_OKAY );
>> +    } while ( rc == X86EMUL_OKAY &&
>> +              (!opt_instruction_limit ||
>> +               (++icount < opt_instruction_limit)) );
> 
> Hmm, if the initalizer of opt_instruction_limit was UINT_MAX, I think
> this wouldn't severely impact results (running 4 billion emulations is
> simply going to take too long) and this expression could be a simple
> comparison.

Yes, we could do that -- we'd have to change the argument parsing code
to handle that case instead, but that's probably a better trade-off.

And I don't have to argue about how having an initializer is easier to
understand what's going on even if it's not strictly necessary. :-)

 -George

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

Re: [PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state

[George Dunlap]

On 10/06/2017 10:57 AM, Jan Beulich wrote:
>>>> On 05.10.17 at 19:08, <george.dunlap@citrix.com> wrote:
>> On 10/04/2017 09:28 AM, Jan Beulich wrote:
>>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>>> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>>>>  };
>>>>  #undef SET
>>>>  
>>>> +static void _set_fpu_state(char *fxsave, bool store)
>>>> +{
>>>> +    if ( cpu_has_fxsr )
>>>> +    {
>>>> +        static union __attribute__((__aligned__(16))) {
>>>> +            char x[464];
>>>
>>> The final part of the save area isn't being written, yes, but is it
>>> really worth saving the few bytes of stack space here, rather than
>>> having the expected 512 as array dimension?
>>
>> So I didn't actually look into this very much; I mainly just hacked at
>> it until it seemed to work.  I copied-and-pasted emul_test_init() from
>> x86_emulate.c (which is where the 464 came from), then copied some
>> scraps of asm from stackoverflow.
> 
> One thing that came to mind in this context: It would perhaps be
> useful to not waste input bytes on the unused portions of the
> save area. Along those lines it may also be worth considering not
> to waste input on the high halves of 64-bit registers as well as
> the high 8 GPRs when emulating 32- or 16-bit mode.

Well with the 'compact' mode we're not wasting anything -- AFL may 'try'
to write to those areas (by setting the <offset> word to those areas),
but it will find that nothing happens and not generate any test cases there.

Even for the high ends of the GPRs, if garbage in those *did* cause a
crash in 32- or 16-bit mode, we'd definitely want to know, wouldn't we?
We'd want to fix it anyway, and we'd need to make sure there wasn't a
way for a guest to trigger that situation with the emulator.

 -George

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

Re: [PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state

[George Dunlap]

On 10/06/2017 07:10 AM, Jan Beulich wrote:
>>>> George Dunlap <george.dunlap@citrix.com> 10/05/17 7:08 PM >>>
>> On 10/04/2017 09:28 AM, Jan Beulich wrote:
>>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>>> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>>>>  };
>>>>  #undef SET
>>>>  
>>>> +static void _set_fpu_state(char *fxsave, bool store)
>>>> +{
>>>> +    if ( cpu_has_fxsr )
>>>> +    {
>>>> +        static union __attribute__((__aligned__(16))) {
>>>> +            char x[464];
>>>
>>> The final part of the save area isn't being written, yes, but is it
>>> really worth saving the few bytes of stack space here, rather than
>>> having the expected 512 as array dimension?
>>
>> So I didn't actually look into this very much; I mainly just hacked at
>> it until it seemed to work.  I copied-and-pasted emul_test_init() from
>> x86_emulate.c (which is where the 464 came from), then copied some
>> scraps of asm from stackoverflow.
> 
> Oh, so it looks like I'm guilty here, as I think it was me who wrote it that
> way there. I have to admit I don't really see why I wanted to save on stack
> consumption there. In any event I'm then fine for you to leave it that way,
> so the two places remain in sync (but I would also be fine if you changed
> it here, and I'd then try to remember to clean it up on the other side).

Well I don't think this function really looks much at all like
emul_test_init(); and I think it makes sense to keep this bit and the
"null" load below identical, so I'll change it to 512.

>>>> +        }
>>>> +        
>>>> +        asm volatile( "fxsave %0" : "=m" (*fxs) );
>>>
>>> This is pretty confusing, the more with the different variable names
>>> used which point to the same piece of memory. You basically store back
>>> into the area you've read from. Is the caller expecting the memory area
>>> to change? Is this being done other than for convenience to not have
>>> another instance of scratch space on the stack? Some comment on the
>>> intentions may be helpful here.
>>
>> Yes, sorry for the different variable names.  I should have done a
>> better clean-up of this patch.
>>
>> As for why it's doing an fxsave after just doing an fxrstor: I had the
>> idea that what came out via fxsave might not be the same as what was
>> written via fxrstor (i.e., the instruction would "interpret" the data),
>> particularly as what went in would be completely random fuzzed state.
>> The idea behind doing the restore / save was to "sanitize" the state in
>> the state struct to look more like real input data.
> 
> Okay, that's what I had guessed. As said, please put this in a comment, the
> more that you've realized this doesn't work all by itself (due to the MXCSR field
> causing #GP when not sanitized _before_ doing the fxrstor). And the restore
> from null then is to pre-init any (theoretical) fields the subsequent restore may
> not touch at all?

Yes; as I said, those two instructions were copied-and-pasted from
stackoverflow (or some other website); and I seem to recall them saying
that architecturally, if a certain amount of the "load data" was zero,
that the unit would simply do a full reset.

>>> The function's parameter name being "store" adds to the confusion,
>>> since what it controls is actually what we call "load" on x86 (or
>>> "restore" following the insn mnemonics).
>>
>> I chose 'store' as the argument name before I realized that fxrstor was
>> "fx restore" and not "fxr store".
>>
>> Do you think 'write' would be suitable?  Names like "restore" or "load"
>> make sense if you're thinking about things from the processor's
>> perspective (as the architects certainly were); but they make less sense
>>from a programmer's perspective, since (to me anyway) it seems like I'm
>> writing to or reading from the FPU unit (rather than loading/restoring
>> or saving).
>>
>> If you don't like 'write' I'll change it to 'restore'.
> 
> "write" is fine, I think, as would be "ro" or "readonly".
> 
>>> And then - what about YMM register state? Other more exotic registers
>>> (like the BND* ones) may indeed not be that relevant to fuzz yet.
>>
>> I can look into that if you want, or if you want to give me some runes
>> to copy in I'm happy to do that as well.
> 
> As that's not as simple as FXSAVE/FXRSTOR (due to first needing to
> discover area sizes) it's perhaps best to simply leave a TODO comment for
> now.

OK.

>>>> @@ -737,6 +780,17 @@ static void setup_state(struct x86_emulate_ctxt *ctxt)
>>>>              printf("Setting cpu_user_regs offset %x\n", offset);
>>>>              continue;
>>>>          }
>>>> +        offset -= sizeof(struct cpu_user_regs);
>>>> +
>>>> +        /* Fuzz fxsave state */
>>>> +        if ( offset < 128 )
>>>> +        {
>>>> +            if ( !input_read(s, s->fxsave + (offset * 4), 4) )
>>>> +                return;
>>>> +            printf("Setting fxsave offset %x\n", offset * 4);
>>>
>>> What's this 32-bit granularity derived from?
>>
>> Just seemed like a good-sized chunk.  Doing it byte-by-byte seemed to be
>> "wasting" input on offsets (as in the input you'd have a 2-byte 'offset'
>> followed by a one-byte bit of data).  This way you have a 2-byte offset
>> and a 4-byte chunk of data that you write.
> 
> Well, ideally individual pieces would be taken all-or-nothing, but due to the
> varying sizes this would be rather cumbersome. So with the comment about
> this being arbitrary add, I think this will be fine for the time being.

OK.

 -George

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

Re: [PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state

[Jan Beulich]

>>> On 06.10.17 at 12:50, <george.dunlap@citrix.com> wrote:
> On 10/06/2017 10:57 AM, Jan Beulich wrote:
>>>>> On 05.10.17 at 19:08, <george.dunlap@citrix.com> wrote:
>>> On 10/04/2017 09:28 AM, Jan Beulich wrote:
>>>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>>>> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>>>>>  };
>>>>>  #undef SET
>>>>>  
>>>>> +static void _set_fpu_state(char *fxsave, bool store)
>>>>> +{
>>>>> +    if ( cpu_has_fxsr )
>>>>> +    {
>>>>> +        static union __attribute__((__aligned__(16))) {
>>>>> +            char x[464];
>>>>
>>>> The final part of the save area isn't being written, yes, but is it
>>>> really worth saving the few bytes of stack space here, rather than
>>>> having the expected 512 as array dimension?
>>>
>>> So I didn't actually look into this very much; I mainly just hacked at
>>> it until it seemed to work.  I copied-and-pasted emul_test_init() from
>>> x86_emulate.c (which is where the 464 came from), then copied some
>>> scraps of asm from stackoverflow.
>> 
>> One thing that came to mind in this context: It would perhaps be
>> useful to not waste input bytes on the unused portions of the
>> save area. Along those lines it may also be worth considering not
>> to waste input on the high halves of 64-bit registers as well as
>> the high 8 GPRs when emulating 32- or 16-bit mode.
> 
> Well with the 'compact' mode we're not wasting anything -- AFL may 'try'
> to write to those areas (by setting the <offset> word to those areas),
> but it will find that nothing happens and not generate any test cases there.
> 
> Even for the high ends of the GPRs, if garbage in those *did* cause a
> crash in 32- or 16-bit mode, we'd definitely want to know, wouldn't we?
> We'd want to fix it anyway, and we'd need to make sure there wasn't a
> way for a guest to trigger that situation with the emulator.

Ah, indeed.

Jan


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

Re: [PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state

[Jan Beulich]

>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>  };
>  #undef SET
>  
> +static void _set_fpu_state(char *fxsave, bool store)
> +{
> +    if ( cpu_has_fxsr )
> +    {
> +        static union __attribute__((__aligned__(16))) {
> +            char x[464];
> +            struct {
> +                uint32_t other[6];
> +                uint32_t mxcsr;
> +                uint32_t mxcsr_mask;
> +                /* ... */
> +            };
> +        } *fxs;
> +
> +        fxs = (typeof(fxs)) fxsave;
> +
> +        if ( store ) {
> +            char null[512] __attribute__((aligned(16))) = { 0 };
> +            asm volatile(" fxrstor %0; "::"m"(*null));
> +            asm volatile(" fxrstor %0; "::"m"(*fxsave));
> +        }
> +        
> +        asm volatile( "fxsave %0" : "=m" (*fxs) );
> +
> +        if ( fxs->mxcsr_mask )
> +            mxcsr_mask = fxs->mxcsr_mask;
> +        else
> +            mxcsr_mask = 0x000ffbf;

Actually - why is this necessary? I.e. why isn't emul_test_init()
setting mxcsr_mask sufficient?

Jan


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

Re: [PATCH v2 13/13] fuzz/x86_emulate: Add an option to limit the number of instructions executed

[Jan Beulich]

>>> On 06.10.17 at 12:40, <george.dunlap@citrix.com> wrote:
> On 10/04/2017 09:28 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>> AFL considers a testcase to be a useful addition not only if there are
>>> tuples exercised by that testcase which were not exercised otherwise,
>>> but also if the *number* of times an individual tuple is exercised
>>> changes significantly; in particular, if the number of the highes bit
>>> changes (i.e., if it is run 1, 2-3, 4-7, 8-15, &c).
>> 
>> Perhaps I simply don't know about AFL (yet) to understand how "highest
>> bit" matters here, or even whose highest bits there's talk of.
> 
> Probably the easiest way to get this would be to read the
> 'technical_details.txt' [1] document about AFL, specifically the section
> "Detecting new behaviors".  The section isn't long, and I'm not sure I
> could explain the situation more concisely than the author has there.

Having read that, I still don't see what "bit" you talk about. The
text there talks about "hit"s - is this perhaps just a typo here?

>>> Unfortunately, one simple way to increase these stats it to execute
>>> the same (or similar) instructions multiple times.
>> 
>> But the change here doesn't look at instruction similarity at all.
> 
> I'm talking about how blind changes AFL makes to the input affect what
> AFL sees at the "output".
> 
> Suppose it has a testcase where instruction A is executed once, and it
> sees tuple N executed twice.  Now suppose it morphs the instruction so
> instruction A is executed twice.  It will now see tuple N executed 4
> times.  This is seen as 'new behavior', and so it will add that as a
> 'new' test case to its set of interesting things to fuzz.  Then suppose
> it morphs one of those so that instruction A is executed four times.
> Tuple N will be executed  8 times, which again is new behavior.  The
> highest tuple count it sees as unique is 128; so in our example, it will
> generate sample inputs up to 64 instructions -- even if the actual path
> through the code for each instruction is identical to the
> single-instruction one.
> 
> A 64-instruction test case will take at least 64x as long to execute as
> a 1-instruction test case; and it will generally also take 64x as long
> to fuzz.  This makes AFL is spending nearly 1000x as much time fuzzing
> that test case as the 1-instruction test case, but for no very good
> reason -- if you can't get actual new behavior we care about out of 2-3
> instructions, you're not going to get it out of 60 instructions.
> 
> IOW, arbitrary numbers of instructions fool AFL into thinking it's found
> something new and interesting when it hasn't.  Limiting the number of
> instructions should in theory keep AFL from getting distracted with test
> cases it thinks are new and unique but aren't.  And we see that for the
> old format, this is true.
> 
> I suspect there's some number of instructions past which we get
> diminishing returns even for the 'compact' format, but since testing
> involves running things for 24 hours, there's also a diminishing returns
> for that kind of optimization. :-)

All understood, yet I still don't understand why you say "the
same (or similar)" when really you only care to limit instruction
count. This is the more that the same insn executed with
different inputs can have dramatically different effects (most
severe case probably being no exception vs exception).

Jan


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

Re: [PATCH v2 13/13] fuzz/x86_emulate: Add an option to limit the number of instructions executed

[George Dunlap]

On 10/06/2017 01:12 PM, Jan Beulich wrote:
>>>> On 06.10.17 at 12:40, <george.dunlap@citrix.com> wrote:
>> On 10/04/2017 09:28 AM, Jan Beulich wrote:
>>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>>> AFL considers a testcase to be a useful addition not only if there are
>>>> tuples exercised by that testcase which were not exercised otherwise,
>>>> but also if the *number* of times an individual tuple is exercised
>>>> changes significantly; in particular, if the number of the highes bit
>>>> changes (i.e., if it is run 1, 2-3, 4-7, 8-15, &c).
>>>
>>> Perhaps I simply don't know about AFL (yet) to understand how "highest
>>> bit" matters here, or even whose highest bits there's talk of.
>>
>> Probably the easiest way to get this would be to read the
>> 'technical_details.txt' [1] document about AFL, specifically the section
>> "Detecting new behaviors".  The section isn't long, and I'm not sure I
>> could explain the situation more concisely than the author has there.
> 
> Having read that, I still don't see what "bit" you talk about. The
> text there talks about "hit"s - is this perhaps just a typo here?

I'm sorry my wording wasn't very precise, but I don't understand why the
examples in parentheses don't communicate what I mean.  Here by "highest
bit" I basically meant, the highest bit which is non-zero.  For 2 and 3,
the number of the highest non-zero bit is 2.  For 4, 5, 6, and 7, the
number of the highest non-zero bit is 3.  For 8-15, the number of the
highest non-zero bit is 4, and so on.

If two test cases touch the exact same branch tuples, but the order of
at least one the counts is different (i.e., if the number of the highest
non-zero bit is different), then AFL considers them as behaving differently.

>>>> Unfortunately, one simple way to increase these stats it to execute
>>>> the same (or similar) instructions multiple times.
>>>
>>> But the change here doesn't look at instruction similarity at all.
>>
>> I'm talking about how blind changes AFL makes to the input affect what
>> AFL sees at the "output".
>>
>> Suppose it has a testcase where instruction A is executed once, and it
>> sees tuple N executed twice.  Now suppose it morphs the instruction so
>> instruction A is executed twice.  It will now see tuple N executed 4
>> times.  This is seen as 'new behavior', and so it will add that as a
>> 'new' test case to its set of interesting things to fuzz.  Then suppose
>> it morphs one of those so that instruction A is executed four times.
>> Tuple N will be executed  8 times, which again is new behavior.  The
>> highest tuple count it sees as unique is 128; so in our example, it will
>> generate sample inputs up to 64 instructions -- even if the actual path
>> through the code for each instruction is identical to the
>> single-instruction one.
>>
>> A 64-instruction test case will take at least 64x as long to execute as
>> a 1-instruction test case; and it will generally also take 64x as long
>> to fuzz.  This makes AFL is spending nearly 1000x as much time fuzzing
>> that test case as the 1-instruction test case, but for no very good
>> reason -- if you can't get actual new behavior we care about out of 2-3
>> instructions, you're not going to get it out of 60 instructions.
>>
>> IOW, arbitrary numbers of instructions fool AFL into thinking it's found
>> something new and interesting when it hasn't.  Limiting the number of
>> instructions should in theory keep AFL from getting distracted with test
>> cases it thinks are new and unique but aren't.  And we see that for the
>> old format, this is true.
>>
>> I suspect there's some number of instructions past which we get
>> diminishing returns even for the 'compact' format, but since testing
>> involves running things for 24 hours, there's also a diminishing returns
>> for that kind of optimization. :-)
> 
> All understood, yet I still don't understand why you say "the
> same (or similar)" when really you only care to limit instruction
> count. This is the more that the same insn executed with
> different inputs can have dramatically different effects (most
> severe case probably being no exception vs exception).

I don't care to limit the instruction count.  I care to restrict AFL
from chasing false leads, thinking that it's discovered "unique" and
"interesting" behavior because it's managed to run an instruction 50
times instead of 5.

One way AFL can chase false leads is by executing *the exact same*
instruction a number of times in a row.  But many instructions which are
*similar* also trigger similar paths; so the same effect could happen
just as well with to instructions that touch nearly the same paths as
with a single instruction.

But I'm not sure what exceptions vs no exceptions has to do with it, so
perhaps I still don't understand you. :-)

 -George

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

Re: [PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[George Dunlap]

On Mon, Sep 25, 2017 at 3:26 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> From: Jan Beulich <jbeulich@suse.com>
>
> fuzz_insn_fetch() is the only data access helper where it is possible
> to see offsets larger than 4Gb in 16- or 32-bit modes, as we leave the
> incoming rIP untouched in the emulator itself. The check is needed here
> as otherwise, after successfully fetching insn bytes, we may end up
> zero-extending EIP soon after complete_insn, which collides with the
> X86EMUL_EXCEPTION-conditional respective ASSERT() in
> x86_emulate_wrapper(). (NB: put_rep_prefix() is what allows
> complete_insn to be reached with rc set to other than X86EMUL_OKAY or
> X86EMUL_DONE. See also commit 53f87c03b4 ["x86emul: generalize
> exception handling for rep_* hooks"].)
>
> Add assert()-s for all other (data) access routines, as effective
> address generation in the emulator ought to guarantee in-range values.
> For them to not trigger, several adjustments to the emulator's address
> calculations are needed: While for DstBitBase it is really mandatory,
> the specification allows for either behavior for two-part accesses.

Something seems to be missing here -- what's mandatory for DstBitBase,
and what are the two  behaviors allowed by the specification for
two-part accesses?

> Observed behavior on real hardware, however, is for such accesses to
> silently wrap at the 2^^32 boundary in other than 64-bit mode, just
> like they do at the 2^^64 boundary in 64-bit mode. While adding
> truncate_ea() invocations there, also convert open coded instances of
> it.
>
> Reported-by: George Dunlap <george.dunlap@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This certainly fixes the issue AFL discovered; and on the whole it
seems to do what the description says, and seems to be pretty
reasonable.

 -George

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

Re: [PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[Jan Beulich]

>>> On 06.10.17 at 17:21, <george.dunlap@citrix.com> wrote:
> On Mon, Sep 25, 2017 at 3:26 PM, George Dunlap <george.dunlap@citrix.com> wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>>
>> fuzz_insn_fetch() is the only data access helper where it is possible
>> to see offsets larger than 4Gb in 16- or 32-bit modes, as we leave the
>> incoming rIP untouched in the emulator itself. The check is needed here
>> as otherwise, after successfully fetching insn bytes, we may end up
>> zero-extending EIP soon after complete_insn, which collides with the
>> X86EMUL_EXCEPTION-conditional respective ASSERT() in
>> x86_emulate_wrapper(). (NB: put_rep_prefix() is what allows
>> complete_insn to be reached with rc set to other than X86EMUL_OKAY or
>> X86EMUL_DONE. See also commit 53f87c03b4 ["x86emul: generalize
>> exception handling for rep_* hooks"].)
>>
>> Add assert()-s for all other (data) access routines, as effective
>> address generation in the emulator ought to guarantee in-range values.
>> For them to not trigger, several adjustments to the emulator's address
>> calculations are needed: While for DstBitBase it is really mandatory,
>> the specification allows for either behavior for two-part accesses.
> 
> Something seems to be missing here -- what's mandatory for DstBitBase,
> and what are the two  behaviors allowed by the specification for
> two-part accesses?

"it" in the sentence above refers to "adjustments" (for DstBitBase
it's just one adjustment, so the singular/plural mismatch is not
really helpful). Similarly "either behavior" refers to the code with
and without the changes done. Would

"Add assert()-s for all other (data) access routines, as effective
 address generation in the emulator ought to guarantee in-range values.
 For them to not trigger, several adjustments to the emulator's address
 calculations are needed: While the DstBitBase one is really mandatory,
 the specification allows for either original or new behavior for two-
 part accesses. Observed behavior on real hardware, however, is for such
 accesses to silently wrap at the 2^^32 boundary in other than 64-bit
 mode, just like they do at the 2^^64 boundary in 64-bit mode, which our
 code is now being brought in line with. While adding truncate_ea()
 invocations there, also convert open coded instances of it."

be any better?

Jan


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

Re: [PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[George Dunlap]

On 10/06/2017 04:54 PM, Jan Beulich wrote:
>>>> On 06.10.17 at 17:21, <george.dunlap@citrix.com> wrote:
>> On Mon, Sep 25, 2017 at 3:26 PM, George Dunlap <george.dunlap@citrix.com> wrote:
>>> From: Jan Beulich <jbeulich@suse.com>
>>>
>>> fuzz_insn_fetch() is the only data access helper where it is possible
>>> to see offsets larger than 4Gb in 16- or 32-bit modes, as we leave the
>>> incoming rIP untouched in the emulator itself. The check is needed here
>>> as otherwise, after successfully fetching insn bytes, we may end up
>>> zero-extending EIP soon after complete_insn, which collides with the
>>> X86EMUL_EXCEPTION-conditional respective ASSERT() in
>>> x86_emulate_wrapper(). (NB: put_rep_prefix() is what allows
>>> complete_insn to be reached with rc set to other than X86EMUL_OKAY or
>>> X86EMUL_DONE. See also commit 53f87c03b4 ["x86emul: generalize
>>> exception handling for rep_* hooks"].)
>>>
>>> Add assert()-s for all other (data) access routines, as effective
>>> address generation in the emulator ought to guarantee in-range values.
>>> For them to not trigger, several adjustments to the emulator's address
>>> calculations are needed: While for DstBitBase it is really mandatory,
>>> the specification allows for either behavior for two-part accesses.
>>
>> Something seems to be missing here -- what's mandatory for DstBitBase,
>> and what are the two  behaviors allowed by the specification for
>> two-part accesses?
> 
> "it" in the sentence above refers to "adjustments" (for DstBitBase
> it's just one adjustment, so the singular/plural mismatch is not
> really helpful). Similarly "either behavior" refers to the code with
> and without the changes done. Would
> 
> "Add assert()-s for all other (data) access routines, as effective
>  address generation in the emulator ought to guarantee in-range values.
>  For them to not trigger, several adjustments to the emulator's address
>  calculations are needed: While the DstBitBase one is really mandatory,
>  the specification allows for either original or new behavior for two-
>  part accesses. Observed behavior on real hardware, however, is for such
>  accesses to silently wrap at the 2^^32 boundary in other than 64-bit
>  mode, just like they do at the 2^^64 boundary in 64-bit mode, which our
>  code is now being brought in line with. While adding truncate_ea()
>  invocations there, also convert open coded instances of it."
> 
> be any better?

Yes, I think so.

One more thing:

> @@ -1249,10 +1249,10 @@ static void __put_rep_prefix(
>
>  /* Clip maximum repetitions so that the index register at most just
wraps. */
>  #define truncate_ea_and_reps(ea, reps, bytes_per_rep) ({
> -    unsigned long todo__, ea__ = truncate_word(ea, ad_bytes);

> +    unsigned long todo__, ea__ = truncate_ea(ea);

>      if ( !(_regs.eflags & X86_EFLAGS_DF) )
> -        todo__ = truncate_word(-(ea), ad_bytes) / (bytes_per_rep);
> -    else if ( truncate_word((ea) + (bytes_per_rep) - 1, ad_bytes) <
ea__ )\
> +        todo__ = truncate_ea(-ea__) / (bytes_per_rep);
> +    else if ( truncate_ea(ea__ + (bytes_per_rep) - 1) < ea__ )

This changes truncate_ea(-ea) to truncate_ea(-truncate_ea(ea)), and
truncate_ea(ea + bpr-1) to truncate_ea(truncate_ea(ea) + bpr-1).  That
sounds like a plausible change, but it's worth checking to see that it
was intentional.

I'm not at the moment able to evaluate the assertion that "the
specification allows for either original or new behavior for two-part
accesses", nor that there is a 1:1 mapping between what this patch
changes and what needs to be changed.

But assuming the above are true (and the change I asked about was
intentional):

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

If that's not good enough to check it in, feel free to give me the
appropriate references to the Intel manual that will allow me to
independently verify those facts, and get rid of my caveat.

 -George

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

Re: [PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[Jan Beulich]

>>> On 06.10.17 at 19:06, <george.dunlap@citrix.com> wrote:
> On 10/06/2017 04:54 PM, Jan Beulich wrote:
>>>>> On 06.10.17 at 17:21, <george.dunlap@citrix.com> wrote:
>>> On Mon, Sep 25, 2017 at 3:26 PM, George Dunlap <george.dunlap@citrix.com> wrote:
> One more thing:
> 
>> @@ -1249,10 +1249,10 @@ static void __put_rep_prefix(
>>
>>  /* Clip maximum repetitions so that the index register at most just wraps. */
>>  #define truncate_ea_and_reps(ea, reps, bytes_per_rep) ({
>> -    unsigned long todo__, ea__ = truncate_word(ea, ad_bytes);
>> +    unsigned long todo__, ea__ = truncate_ea(ea);
>>      if ( !(_regs.eflags & X86_EFLAGS_DF) )
>> -        todo__ = truncate_word(-(ea), ad_bytes) / (bytes_per_rep);
>> -    else if ( truncate_word((ea) + (bytes_per_rep) - 1, ad_bytes) < ea__ )\
>> +        todo__ = truncate_ea(-ea__) / (bytes_per_rep);
>> +    else if ( truncate_ea(ea__ + (bytes_per_rep) - 1) < ea__ )
> 
> This changes truncate_ea(-ea) to truncate_ea(-truncate_ea(ea)), and
> truncate_ea(ea + bpr-1) to truncate_ea(truncate_ea(ea) + bpr-1).  That
> sounds like a plausible change, but it's worth checking to see that it
> was intentional.

The main goal here was to eliminate the multiple evaluation of
the macro argument plus the open-coding of truncate_ea(). With
truncate_ea(truncate_ea(x)) == truncate_ea(x) and
truncate_ea(-truncate_ea(x)) == truncate_ea(-x) there's no
change in net result. So yes, the change was intentional.

> I'm not at the moment able to evaluate the assertion that "the
> specification allows for either original or new behavior for two-part
> accesses", nor that there is a 1:1 mapping between what this patch
> changes and what needs to be changed.

Section "Limit Checking" in volume 3 has

"When the effective limit is FFFFFFFFH (4 GBytes), these accesses
 may or may not cause the indicated exceptions. Behavior is
 implementation-specific and may vary from one execution to another."

Additionally section "Segment Wraparound" in volume 3 has

"The behavior when executing near the limit of a 4-GByte selector
 (limit = FFFFFFFFH) is different between the Pentium Pro and the
 Pentium 4 family of processors. On the Pentium Pro, instructions
 which cross the limit -- for example, a two byte instruction such as
 INC EAX that is encoded as FFH C0H starting exactly at the limit
 faults for a segment violation (a one byte instruction at FFFFFFFFH
 does not cause an exception). Using the Pentium 4 microprocessor
 family, neither of these situations causes a fault."

> But assuming the above are true (and the change I asked about was
> intentional):
> 
> Reviewed-by: George Dunlap <george.dunlap@citrix.com>

Thanks.

> If that's not good enough to check it in, feel free to give me the
> appropriate references to the Intel manual that will allow me to
> independently verify those facts, and get rid of my caveat.

The patch still needs Andrew's ack before it can go in.

Jan


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

Re: [PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[Andrew Cooper]

On 25/09/17 15:26, George Dunlap wrote:
> From: Jan Beulich <jbeulich@suse.com>
>
> fuzz_insn_fetch() is the only data access helper where it is possible
> to see offsets larger than 4Gb in 16- or 32-bit modes, as we leave the
> incoming rIP untouched in the emulator itself.

Is it reasonable to tolerate this?  AFAICT, this is only because we have
unsanitised fuzzing data in the input?

VMX will fail a vmentry if any of the upper 32 %rip bits are set if CS.L
is clear.  (OTOH, the next check in the list is the canonical check,
which is bogus on vmentry, so maybe this isn't a good example to take.)

>  The check is needed here
> as otherwise, after successfully fetching insn bytes, we may end up
> zero-extending EIP soon after complete_insn, which collides with the
> X86EMUL_EXCEPTION-conditional respective ASSERT() in
> x86_emulate_wrapper().

In light of this, even more reason that the wrapper should gain:

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
b/xen/arch/x86/x86_emulate/x86_emulate.c
index a68676c..4845cad 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -7944,6 +7944,8 @@ int x86_emulate_wrapper(
 
     if ( mode_64bit() )
         ASSERT(ctxt->lma);
+    else
+        ASSERT((orig_ip >> 32) == 0);
 
     rc = x86_emulate(ctxt, ops);
 


>  (NB: put_rep_prefix() is what allows
> complete_insn to be reached with rc set to other than X86EMUL_OKAY or
> X86EMUL_DONE. See also commit 53f87c03b4 ["x86emul: generalize
> exception handling for rep_* hooks"].)
>
> Add assert()-s for all other (data) access routines, as effective
> address generation in the emulator ought to guarantee in-range values.
> For them to not trigger, several adjustments to the emulator's address
> calculations are needed: While for DstBitBase it is really mandatory,
> the specification allows for either behavior for two-part accesses.

I can't parse this sentence.  What does the "it" following DstBitBase
refer to, and which two things do "either behaviours" refer to?

> Observed behavior on real hardware, however, is for such accesses to
> silently wrap at the 2^^32 boundary in other than 64-bit mode, just
> like they do at the 2^^64 boundary in 64-bit mode. While adding
> truncate_ea() invocations there, also convert open coded instances of
> it.
>
> Reported-by: George Dunlap <george.dunlap@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> ---
>  tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 32 ++++++++++++++++++++++---
>  xen/arch/x86/x86_emulate/x86_emulate.c          | 22 +++++++++--------
>  2 files changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> index a2329f84a5..105145e9f9 100644
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -139,7 +139,18 @@ static int fuzz_read(
>      struct x86_emulate_ctxt *ctxt)
>  {
>      /* Reads expected for all user and system segments. */
> -    assert(is_x86_user_segment(seg) || is_x86_system_segment(seg));
> +    if ( is_x86_user_segment(seg) )
> +        assert(ctxt->addr_size == 64 || !(offset >> 32));
> +    else if ( seg == x86_seg_tr )
> +        /*
> +         * The TSS is special in that accesses below the segment base are
> +         * possible, as the Interrupt Redirection Bitmap starts 32 bytes
> +         * ahead of the I/O Bitmap, regardless of the value of the latter.
> +         */
> +        assert((long)offset < 0 ? (long)offset > -32 : !(offset >> 17));
> +    else
> +        assert(is_x86_system_segment(seg) &&
> +               (ctxt->lma ? offset <= 0x10007 : !(offset >> 16)));

Where do the extra 7 bytes come from in long mode?  I'm not aware of an
architectural way to make misaligned offsets into the LDT/GDT/IDT.

~Andrew

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

Re: [PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[Jan Beulich]

>>> On 09.10.17 at 14:54, <andrew.cooper3@citrix.com> wrote:
> On 25/09/17 15:26, George Dunlap wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>>
>> fuzz_insn_fetch() is the only data access helper where it is possible
>> to see offsets larger than 4Gb in 16- or 32-bit modes, as we leave the
>> incoming rIP untouched in the emulator itself.
> 
> Is it reasonable to tolerate this?  AFAICT, this is only because we have
> unsanitised fuzzing data in the input?

Yes, that's what I recall, i.e.

> VMX will fail a vmentry if any of the upper 32 %rip bits are set if CS.L
> is clear.  (OTOH, the next check in the list is the canonical check,
> which is bogus on vmentry, so maybe this isn't a good example to take.)

... this has nothing to do with the former. The new %rip will
always be clipped.

>>  The check is needed here
>> as otherwise, after successfully fetching insn bytes, we may end up
>> zero-extending EIP soon after complete_insn, which collides with the
>> X86EMUL_EXCEPTION-conditional respective ASSERT() in
>> x86_emulate_wrapper().
> 
> In light of this, even more reason that the wrapper should gain:
> 
> diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
> b/xen/arch/x86/x86_emulate/x86_emulate.c
> index a68676c..4845cad 100644
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -7944,6 +7944,8 @@ int x86_emulate_wrapper(
>  
>      if ( mode_64bit() )
>          ASSERT(ctxt->lma);
> +    else
> +        ASSERT((orig_ip >> 32) == 0);
>  
>      rc = x86_emulate(ctxt, ops);

I'm not convinced - the emulator works fine without this check, and
I don't think we should assert on inputs coming from various sources.
If anything I could see us return UNHANDLEABLE in that case.

>>  (NB: put_rep_prefix() is what allows
>> complete_insn to be reached with rc set to other than X86EMUL_OKAY or
>> X86EMUL_DONE. See also commit 53f87c03b4 ["x86emul: generalize
>> exception handling for rep_* hooks"].)
>>
>> Add assert()-s for all other (data) access routines, as effective
>> address generation in the emulator ought to guarantee in-range values.
>> For them to not trigger, several adjustments to the emulator's address
>> calculations are needed: While for DstBitBase it is really mandatory,
>> the specification allows for either behavior for two-part accesses.
> 
> I can't parse this sentence.  What does the "it" following DstBitBase
> refer to, and which two things do "either behaviours" refer to?

See the discussion we've have with George. The replacement text
now reads

"Add assert()-s for all other (data) access routines, as effective
 address generation in the emulator ought to guarantee in-range values.
 For them to not trigger, several adjustments to the emulator's address
 calculations are needed: While the DstBitBase one is really mandatory,
 the specification allows for either original or new behavior for two-
 part accesses. Observed behavior on real hardware, however, is for such
 accesses to silently wrap at the 2^^32 boundary in other than 64-bit
 mode, just like they do at the 2^^64 boundary in 64-bit mode, which our
 code is now being brought in line with. While adding truncate_ea()
 invocations there, also convert open coded instances of it."

>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> @@ -139,7 +139,18 @@ static int fuzz_read(
>>      struct x86_emulate_ctxt *ctxt)
>>  {
>>      /* Reads expected for all user and system segments. */
>> -    assert(is_x86_user_segment(seg) || is_x86_system_segment(seg));
>> +    if ( is_x86_user_segment(seg) )
>> +        assert(ctxt->addr_size == 64 || !(offset >> 32));
>> +    else if ( seg == x86_seg_tr )
>> +        /*
>> +         * The TSS is special in that accesses below the segment base are
>> +         * possible, as the Interrupt Redirection Bitmap starts 32 bytes
>> +         * ahead of the I/O Bitmap, regardless of the value of the latter.
>> +         */
>> +        assert((long)offset < 0 ? (long)offset > -32 : !(offset >> 17));
>> +    else
>> +        assert(is_x86_system_segment(seg) &&
>> +               (ctxt->lma ? offset <= 0x10007 : !(offset >> 16)));
> 
> Where do the extra 7 bytes come from in long mode?  I'm not aware of an
> architectural way to make misaligned offsets into the LDT/GDT/IDT.

It's 8 extra bytes - when we read a gate descriptor, the upper
half may exceed the standard 64k limit. This is part of why the
title says "rudimentary".

Jan

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

Re: [PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[Andrew Cooper]

On 09/10/17 14:26, Jan Beulich wrote:
>
>>>  (NB: put_rep_prefix() is what allows
>>> complete_insn to be reached with rc set to other than X86EMUL_OKAY or
>>> X86EMUL_DONE. See also commit 53f87c03b4 ["x86emul: generalize
>>> exception handling for rep_* hooks"].)
>>>
>>> Add assert()-s for all other (data) access routines, as effective
>>> address generation in the emulator ought to guarantee in-range values.
>>> For them to not trigger, several adjustments to the emulator's address
>>> calculations are needed: While for DstBitBase it is really mandatory,
>>> the specification allows for either behavior for two-part accesses.
>> I can't parse this sentence.  What does the "it" following DstBitBase
>> refer to, and which two things do "either behaviours" refer to?
> See the discussion we've have with George. The replacement text
> now reads
>
> "Add assert()-s for all other (data) access routines, as effective
>  address generation in the emulator ought to guarantee in-range values.
>  For them to not trigger, several adjustments to the emulator's address
>  calculations are needed: While the DstBitBase one is really mandatory,
>  the specification allows for either original or new behavior for two-
>  part accesses. Observed behavior on real hardware, however, is for such
>  accesses to silently wrap at the 2^^32 boundary in other than 64-bit
>  mode, just like they do at the 2^^64 boundary in 64-bit mode, which our
>  code is now being brought in line with. While adding truncate_ea()
>  invocations there, also convert open coded instances of it."

Ok.  Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v2 12/13] fuzz/x86_emulate: Set and fuzz more CPU state

[George Dunlap]

On 10/06/2017 12:56 PM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>>  };
>>  #undef SET
>>  
>> +static void _set_fpu_state(char *fxsave, bool store)
>> +{
>> +    if ( cpu_has_fxsr )
>> +    {
>> +        static union __attribute__((__aligned__(16))) {
>> +            char x[464];
>> +            struct {
>> +                uint32_t other[6];
>> +                uint32_t mxcsr;
>> +                uint32_t mxcsr_mask;
>> +                /* ... */
>> +            };
>> +        } *fxs;
>> +
>> +        fxs = (typeof(fxs)) fxsave;
>> +
>> +        if ( store ) {
>> +            char null[512] __attribute__((aligned(16))) = { 0 };
>> +            asm volatile(" fxrstor %0; "::"m"(*null));
>> +            asm volatile(" fxrstor %0; "::"m"(*fxsave));
>> +        }
>> +        
>> +        asm volatile( "fxsave %0" : "=m" (*fxs) );
>> +
>> +        if ( fxs->mxcsr_mask )
>> +            mxcsr_mask = fxs->mxcsr_mask;
>> +        else
>> +            mxcsr_mask = 0x000ffbf;
> 
> Actually - why is this necessary? I.e. why isn't emul_test_init()
> setting mxcsr_mask sufficient?

This is me not understanding what's going on.  I've removed this bit,
and modified this function to do the 'sanitation' -- to mask off mxcsr
before doing the fxrstor (and removed the change from "sanitize_input").

 -George

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