Re: [PATCH v3 5/7] tools/fuzz: introduce x86 instruction emulator target

[Wei Liu <wei.liu2@citrix.com>]

On Mon, Dec 12, 2016 at 02:58:39AM -0700, Jan Beulich wrote:
> >>> On 12.12.16 at 10:28, <wei.liu2@citrix.com> wrote:
> > Instruction emulator fuzzing code is from code previous written by
> > Andrew and George. Adapted to llvm fuzzer and hook up the build system.
> > 
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > 
> > v3:
> > 1. coding style fix
> > 2. share more code
> > 3. exit when stack can't be made executable
> > ---
> >  .gitignore                                         |   1 +
> >  tools/fuzz/x86_instruction_emulator/Makefile       |  31 ++++
> >  .../x86-insn-emulator-fuzzer.c                     | 195 +++++++++++++++++++++
> >  3 files changed, 227 insertions(+)
> >  create mode 100644 tools/fuzz/x86_instruction_emulator/Makefile
> >  create mode 100644 
> > tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
> > 
> > diff --git a/.gitignore b/.gitignore
> > index a2f34a1..d507243 100644
> > --- a/.gitignore
> > +++ b/.gitignore
> > @@ -145,6 +145,7 @@ tools/flask/utils/flask-loadpolicy
> >  tools/flask/utils/flask-setenforce
> >  tools/flask/utils/flask-set-bool
> >  tools/flask/utils/flask-label-pci
> > +tools/fuzz/x86_instruction_emulator/x86_emulate*
> >  tools/helpers/_paths.h
> >  tools/helpers/init-xenstore-domain
> >  tools/helpers/xen-init-dom0
> > diff --git a/tools/fuzz/x86_instruction_emulator/Makefile 
> > b/tools/fuzz/x86_instruction_emulator/Makefile
> > new file mode 100644
> > index 0000000..2b147ac
> > --- /dev/null
> > +++ b/tools/fuzz/x86_instruction_emulator/Makefile
> > @@ -0,0 +1,31 @@
> > +XEN_ROOT=$(CURDIR)/../../..
> > +include $(XEN_ROOT)/tools/Rules.mk
> > +
> > +x86-instruction-emulator-fuzzer-all: x86-insn-emulator.a x86-insn-emulator-fuzzer.o
> > +
> > +x86_emulate/x86_emulate.c x86_emulate/x86_emulate.h:
> > +	[ -L x86_emulate ] || ln -sf $(XEN_ROOT)/xen/arch/x86/x86_emulate .
> > +
> > +x86_emulate.c x86_emulate.h: %:
> > +	[ -L $* ] || ln -sf $(XEN_ROOT)/tools/tests/x86_emulator/$*
> > +
> > +CFLAGS += $(CFLAGS_xeninclude)
> > +
> > +x86_emulate.o: x86_emulate.c x86_emulate.h x86_emulate/x86_emulate.c x86_emulate/x86_emulate.h
> 
> Perhaps worthwhile shortening this to
> 
> x86_emulate.o: x86_emulate.[ch] x86_emulate/x86_emulate.[ch]
> 
> ?

Done.

> 
> > --- /dev/null
> > +++ b/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
> > @@ -0,0 +1,195 @@
> > +#include <assert.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <inttypes.h>
> > +#include <limits.h>
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/types.h>
> > +#include <sys/stat.h>
> > +#include <sys/mman.h>
> > +#include <unistd.h>
> > +#include <xen/xen.h>
> > +
> > +#include "x86_emulate.h"
> > +
> > +static unsigned char data[4096];
> > +static unsigned int data_index = 0;
> 
> Pointless initializer.
> 

Done.

> > +static unsigned int data_max;
> > +
> > +static int data_read(const char *why, void *dst, unsigned int bytes)
> > +{
> > +    unsigned i;
> 
> Please don't omit the "int" here (and in a few more places below)
> when basically everywhere else it is present.
> 

Done.

> > +    if ( data_index + bytes > data_max )
> > +        return X86EMUL_EXCEPTION;
> > +
> > +    memcpy(dst,  data+data_index, bytes);
> 
> Blanks around binary operators please (more further down).
> 

Done.

> > +    data_index += bytes;
> > +
> > +    printf("%s: ", why);
> > +    for ( i = 0; i < bytes; i++ )
> > +        printf(" %02x", (unsigned int)*(unsigned char *)(dst+i));
> 
> Is the left most cast really needed here?
> 

No. I've deleted that.

> > +int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
> > +{
> > +    bool stack_exec;
> > +    struct cpu_user_regs regs = {};
> > +    struct x86_emulate_ctxt ctxt =
> > +        {
> > +            .regs = &regs,
> > +            .addr_size = 8 * sizeof(void *),
> > +            .sp_size = 8 * sizeof(void *),
> > +        };
> > +
> 
> Stray blank line. The indentation of the initializer above also looks
> a little unusual.
> 

Fixed.

> > +    unsigned nr = 0;
> > +    int rc;
> > +    unsigned x;
> > +    const uint8_t *p = data_p;
> > +
> > +    stack_exec = emul_test_make_stack_executable();
> > +    if ( !stack_exec )
> > +    {
> > +        printf("Warning: Stack could not be made executable (%d).\n", errno);
> > +        exit(1);
> > +    }
> > +
> > +    /* Reset all global states */
> 
> DYM "state"?
> 

I mean "states". There are three states we need to reset.

> > +    memset(data, 0, sizeof(data));
> > +    data_index = 0;
> > +    data_max = 0;
> > +
> > +    nr = size < sizeof(regs) ? size : sizeof(regs);
> > +
> > +    memcpy(&regs, p, nr);
> > +    p += sizeof(regs);
> > +    nr += sizeof(regs);
> 
> I think this second += wants to be dropped, considering how nr
> gets set above and used below.
> 
> > +    if ( nr <= size )
> 
> < would seem more natural here.

Yes, you're right in both places.

> 
> > +    {
> > +        memcpy(data, p, size - nr);
> > +        data_max = size - nr;
> > +    }
> > +
> > +    ctxt.force_writeback = 0;
> 
> false

Done.

> 
> > +    /* Zero 'private' entries */
> 
> s/entries/fields/ ?
> 

Done.

> > +    regs.error_code = 0;
> > +    regs.entry_vector = 0;
> > +
> > +    /* Use the upper bits of regs.eip to determine addr_size */
> > +    x = (regs.rip >> ADDR_SIZE_SHIFT) & 0x3;
> 
> This won't build as 32-bit code. If that's intentional, then I think
> this would better be enforced in the Makefile (rather than
> surfacing a compile error here).
> 

Good catch. I think this test case is still preliminary. TBH I haven't
paid much attention to the working of this test target, other than
pulling everything together to work.

I think long term we do need to determine what to do with 32 bit build,
but I would wait until George to come back because he wrote this
snippet.

For now I will disable 32bit build in Makefile.

> > +    if (x == 3)

I also fix this instance to add spaces in ().


---8<---
From 83b7381080aafc3f2fb35ba589715694f847f73a Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Thu, 8 Dec 2016 12:09:54 +0000
Subject: [PATCH] tools/fuzz: introduce x86 instruction emulator target

Instruction emulator fuzzing code is from code previous written by
Andrew and George. Adapt it to llvm fuzzer and hook up the build system.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

v4:
1. more coding style fixes and bug fixes
2. only do 64 bit build

v3:
1. coding style fix
2. share more code
3. exit when stack can't be made executable
---
 .gitignore                                         |   1 +
 tools/fuzz/x86_instruction_emulator/Makefile       |  36 ++++
 .../x86-insn-emulator-fuzzer.c                     | 190 +++++++++++++++++++++
 3 files changed, 227 insertions(+)
 create mode 100644 tools/fuzz/x86_instruction_emulator/Makefile
 create mode 100644 tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c

diff --git a/.gitignore b/.gitignore
index a2f34a1..d507243 100644
--- a/.gitignore
+++ b/.gitignore
@@ -145,6 +145,7 @@ tools/flask/utils/flask-loadpolicy
 tools/flask/utils/flask-setenforce
 tools/flask/utils/flask-set-bool
 tools/flask/utils/flask-label-pci
+tools/fuzz/x86_instruction_emulator/x86_emulate*
 tools/helpers/_paths.h
 tools/helpers/init-xenstore-domain
 tools/helpers/xen-init-dom0
diff --git a/tools/fuzz/x86_instruction_emulator/Makefile b/tools/fuzz/x86_instruction_emulator/Makefile
new file mode 100644
index 0000000..6e68df7
--- /dev/null
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -0,0 +1,36 @@
+XEN_ROOT=$(CURDIR)/../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+.PHONY: x86-instruction-emulator-fuzzer-all
+ifeq ($(CONFIG_X86_64),y)
+x86-instruction-emulator-fuzzer-all: x86-insn-emulator.a x86-insn-emulator-fuzzer.o
+else
+x86-instruction-emulator-fuzzer-all:
+endif
+
+x86_emulate/x86_emulate.c x86_emulate/x86_emulate.h:
+	[ -L x86_emulate ] || ln -sf $(XEN_ROOT)/xen/arch/x86/x86_emulate .
+
+x86_emulate.c x86_emulate.h: %:
+	[ -L $* ] || ln -sf $(XEN_ROOT)/tools/tests/x86_emulator/$*
+
+CFLAGS += $(CFLAGS_xeninclude)
+
+x86_emulate.o: x86_emulate.[ch] x86_emulate/x86_emulate.[ch]
+
+x86-insn-emulator.a: x86_emulate.o
+	$(AR) rc $@ $^
+
+x86-insn-emulator-fuzzer.o: x86-insn-emulator-fuzzer.c
+
+# Common targets
+.PHONY: all
+all: x86-instruction-emulator-fuzzer-all
+
+.PHONY: distclean
+distclean: clean
+	rm -f x86_emulate x86_emulate.c x86_emulate.h
+
+.PHONY: clean
+clean:
+	rm -f *.a *.o
diff --git a/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c b/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
new file mode 100644
index 0000000..94ec311
--- /dev/null
+++ b/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
@@ -0,0 +1,190 @@
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <limits.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <unistd.h>
+#include <xen/xen.h>
+
+#include "x86_emulate.h"
+
+static unsigned char data[4096];
+static unsigned int data_index;
+static unsigned int data_max;
+
+static int data_read(const char *why, void *dst, unsigned int bytes)
+{
+    unsigned int i;
+
+    if ( data_index + bytes > data_max )
+        return X86EMUL_EXCEPTION;
+
+    memcpy(dst,  data + data_index, bytes);
+    data_index += bytes;
+
+    printf("%s: ", why);
+    for ( i = 0; i < bytes; i++ )
+        printf(" %02x", *(unsigned char *)(dst + i));
+    printf("\n");
+
+    return X86EMUL_OKAY;
+}
+
+static int fuzz_read(
+    unsigned int seg,
+    unsigned long offset,
+    void *p_data,
+    unsigned int bytes,
+    struct x86_emulate_ctxt *ctxt)
+{
+    return data_read("read", p_data, bytes);
+}
+
+static int fuzz_fetch(
+    unsigned int seg,
+    unsigned long offset,
+    void *p_data,
+    unsigned int bytes,
+    struct x86_emulate_ctxt *ctxt)
+{
+    return data_read("fetch", p_data, bytes);
+}
+
+static int fuzz_write(
+    unsigned int seg,
+    unsigned long offset,
+    void *p_data,
+    unsigned int bytes,
+    struct x86_emulate_ctxt *ctxt)
+{
+    return X86EMUL_OKAY;
+}
+
+static int fuzz_cmpxchg(
+    unsigned int seg,
+    unsigned long offset,
+    void *old,
+    void *new,
+    unsigned int bytes,
+    struct x86_emulate_ctxt *ctxt)
+{
+    return X86EMUL_OKAY;
+}
+
+static struct x86_emulate_ops fuzz_emulops = {
+    .read       = fuzz_read,
+    .insn_fetch = fuzz_fetch,
+    .write      = fuzz_write,
+    .cmpxchg    = fuzz_cmpxchg,
+    .cpuid      = emul_test_cpuid,
+    .read_cr    = emul_test_read_cr,
+    .get_fpu    = emul_test_get_fpu,
+};
+
+#define CANONICALIZE(x)                                   \
+    do {                                                  \
+        uint64_t _y = (x);                                \
+        if ( _y & (1ULL << 47) )                          \
+            _y |= (~0ULL) << 48;                          \
+        else                                              \
+            _y &= (1ULL << 48)-1;                         \
+        printf("Canonicalized %" PRIx64 " to %" PRIx64 "\n", x, _y);    \
+        (x) = _y;                                       \
+    } while( 0 )
+
+#define ADDR_SIZE_SHIFT 60
+#define ADDR_SIZE_64 (2ULL << ADDR_SIZE_SHIFT)
+#define ADDR_SIZE_32 (1ULL << ADDR_SIZE_SHIFT)
+#define ADDR_SIZE_16 (0)
+
+int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
+{
+    bool stack_exec;
+    struct cpu_user_regs regs = {};
+    struct x86_emulate_ctxt ctxt = {
+        .regs = &regs,
+        .addr_size = 8 * sizeof(void *),
+        .sp_size = 8 * sizeof(void *),
+    };
+    unsigned int nr = 0;
+    int rc;
+    unsigned int x;
+    const uint8_t *p = data_p;
+
+    stack_exec = emul_test_make_stack_executable();
+    if ( !stack_exec )
+    {
+        printf("Warning: Stack could not be made executable (%d).\n", errno);
+        return 1;
+    }
+
+    /* Reset all global states */
+    memset(data, 0, sizeof(data));
+    data_index = 0;
+    data_max = 0;
+
+    nr = size < sizeof(regs) ? size : sizeof(regs);
+
+    memcpy(&regs, p, nr);
+    p += sizeof(regs);
+
+    if ( nr < size )
+    {
+        memcpy(data, p, size - nr);
+        data_max = size - nr;
+    }
+
+    ctxt.force_writeback = false;
+
+    /* Zero 'private' fields */
+    regs.error_code = 0;
+    regs.entry_vector = 0;
+
+    /* Use the upper bits of regs.eip to determine addr_size */
+    x = (regs.rip >> ADDR_SIZE_SHIFT) & 0x3;
+    if ( x == 3 )
+        x = 2;
+    ctxt.addr_size = 16 << x;
+    printf("addr_size: %d\n", ctxt.addr_size);
+
+    /* Use the upper bit of regs.rsp to determine sp_size (if appropriate) */
+    if ( ctxt.addr_size == 64 )
+        ctxt.sp_size = 64;
+    else
+    {
+        /* If addr_size isn't 64-bits, sp_size can only be 16 or 32 bits */
+        x = (regs.rsp >> ADDR_SIZE_SHIFT) & 0x1;
+        ctxt.sp_size = 16 << x;
+    }
+    printf("sp_size: %d\n", ctxt.sp_size);
+    CANONICALIZE(regs.rip);
+    CANONICALIZE(regs.rsp);
+    CANONICALIZE(regs.rbp);
+
+    /* Zero all segments for now */
+    regs.cs = regs.ss = regs.es = regs.ds = regs.fs = regs.gs = 0;
+
+    do {
+        rc = x86_emulate(&ctxt, &fuzz_emulops);
+        printf("Emulation result: %d\n", rc);
+    } while ( rc == X86EMUL_OKAY );
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.1.4


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