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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.17 at 16:26, <george.dunlap@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel