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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.12.16 at 10:28, <wei.liu2@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> ---
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> 
> 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]

?

> --- /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.

> +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.

> +    if ( data_index + bytes > data_max )
> +        return X86EMUL_EXCEPTION;
> +
> +    memcpy(dst,  data+data_index, bytes);

Blanks around binary operators please (more further down).

> +    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?

> +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.

> +    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"?

> +    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.

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

false

> +    /* Zero 'private' entries */

s/entries/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;

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).

> +    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;
> +    }

Pointless braces.

Jan

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