|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 5/7] fuzz/x86emul: update fuzzer
>>> On 25.01.17 at 16:44, <wei.liu2@xxxxxxxxxx> wrote:
> --- a/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
> +++ b/tools/fuzz/x86_instruction_emulator/x86-insn-emulator-fuzzer.c
> @@ -16,26 +16,75 @@
>
> #include "x86_emulate.h"
>
> -static unsigned char data[4096];
> +#include "../../../xen/include/asm-x86/msr-index.h"
> +
> +#ifndef offsetof
> +#define offsetof(t,m) ((size_t)&(((t *)0)->m))
> +#endif
Why can't you include the standard header for this?
> +#define MSR_INDEX_MAX 16
> +
> +#define SEG_MAX x86_seg_none
> +
> +struct input_struct {
> + unsigned cr[5];
unsigned long?
> + uint64_t msr[MSR_INDEX_MAX];
> + struct cpu_user_regs regs;
> + struct segment_register segments[SEG_MAX+1];
Either SEG_MAX needs to be x86_seg_none - 1, or it is slightly
mis-named (should be SEG_NUM) and the +1 here is not needed.
> +int maybe_fail(const char *why, bool exception)
> +{
> + int rc;
> +
> + if ( data_index + 1 > data_max )
The naming issue makes this look wrong too - I think you again mean
data_num or data_amount, not data_max.
> + return X86EMUL_EXCEPTION;
> + else
> + {
> + if ( input.data[data_index] > 0xc )
> + rc = X86EMUL_EXCEPTION;
> + else if ( input.data[data_index] > 0x8 )
> + rc = X86EMUL_UNHANDLEABLE;
How were these numbers chosen?
> + else
> + rc = X86EMUL_OKAY;
> + data_index++;
> + }
> +
> + if ( rc == X86EMUL_EXCEPTION && !exception )
> + rc = X86EMUL_OKAY;
This could do with a comment explaining the intention.
> + printf("maybe_fail %s: %d\n", why, rc);
> +
> + return rc;
> +}
> +
> static int data_read(const char *why, void *dst, unsigned int bytes)
> {
> unsigned int i;
> + int rc;
>
> if ( data_index + bytes > data_max )
> return X86EMUL_EXCEPTION;
> + else
> + rc = maybe_fail(why, true);
No need for "else".
> @@ -48,14 +97,71 @@ static int fuzz_read(
> return data_read("read", p_data, bytes);
> }
>
> -static int fuzz_fetch(
> +static int fuzz_read_io(
> + unsigned int port,
> + unsigned int bytes,
> + unsigned long *val,
> + struct x86_emulate_ctxt *ctxt)
> +{
> + return data_read("read_io", val, bytes);
> +}
> +
> +static int fuzz_insn_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);
> + return data_read("insn_fetch", p_data, bytes);
Why is "fetch" not good enough? It looks like there may be a lot of
output, so any compaction opportunity may be worthwhile to use.
> +static int fuzz_rep_movs(
> + enum x86_segment src_seg,
> + unsigned long src_offset,
> + enum x86_segment dst_seg,
> + unsigned long dst_offset,
> + unsigned int bytes_per_rep,
> + unsigned long *reps,
> + struct x86_emulate_ctxt *ctxt)
> +{
> + int rc;
> + unsigned long bytes_read;
> + rc = data_read("rep_ins", &bytes_read, sizeof(bytes_read));
"rep_movs"
> +static int fuzz_rep_stos(
> + void *p_data,
> + enum x86_segment seg,
> + unsigned long offset,
> + unsigned int bytes_per_rep,
> + unsigned long *reps,
> + struct x86_emulate_ctxt *ctxt)
> +{
> + return maybe_fail("rep_stos", true);
> }
Perhaps better moved next to rep_movs().
> +static int fuzz_cpuid(
> + uint32_t leaf,
> + uint32_t subleaf,
> + struct cpuid_leaf *res,
> + struct x86_emulate_ctxt *ctxt)
> +{
> + int rc;
> +
> + rc = maybe_fail("cpuid", true);
> +
> + if ( rc == X86EMUL_OKAY )
> + emul_test_cpuid(leaf, subleaf, res, ctxt);
> +
> + return rc;
> +}
Careful here: ->cpuid() is documented to only be allowed to fail with
X86EMUL_EXCEPTION.
> +static int fuzz_read_segment(
> + enum x86_segment seg,
> + struct segment_register *reg,
> + struct x86_emulate_ctxt *ctxt)
> +{
> + int rc;
> +
> + if ( seg > SEG_MAX )
> + return X86EMUL_UNHANDLEABLE;
> +
> + rc = maybe_fail("read_segment", true);
> +
> + if ( rc == X86EMUL_OKAY )
> + memcpy(reg, input.segments+seg, sizeof(struct segment_register));
> +
> + return rc;
> +}
This continues from the earlier comment: x86_seg_none must not
make it here (i.e. should by included in the early bailing).
> +static int fuzz_write_segment(
> + enum x86_segment seg,
> + const struct segment_register *reg,
> + struct x86_emulate_ctxt *ctxt)
> +{
> + int rc;
> +
> + if ( seg > SEG_MAX )
> + return X86EMUL_UNHANDLEABLE;
> +
> + rc = maybe_fail("write_segment", true);
> +
> + if ( rc == X86EMUL_OKAY )
> + memcpy(input.segments+seg, reg, sizeof(struct segment_register));
> +
> + return rc;
> +}
Even more so here then.
> +static int fuzz_read_cr(
> + unsigned int reg,
> + unsigned long *val,
> + struct x86_emulate_ctxt *ctxt)
> +{
> + int rc;
> +
> + if ( reg > 5 )
ARRAY_SIZE()
> +static int fuzz_write_cr(
> + unsigned int reg,
> + unsigned long val,
> + struct x86_emulate_ctxt *ctxt)
> +{
> + int rc;
> +
> + if ( reg > 5 )
Same here.
> +int msr_index[MSR_INDEX_MAX] = {
static unsigned int (and maybe also const, I didn't check whether it
gets written to).
> +static int fuzz_read_msr_(
Please use a leading rather than a trailing underscore (unless there's
a reason for you doing it that way).
> +static void setup_fpu_exception_handler(void)
> +{
> + /* FIXME - just disable exceptions for now */
> + unsigned long a;
> +
> + asm volatile ( "fnclex");
> + a=0x3f;
> + asm volatile ( "fldcw %0" :: "m" (a));
> + a=0x1f80;
> + asm volatile ( "ldmxcsr %0" :: "m" (a) );
I don't see what you need "a" for in this function. And I also wonder
how the FCW value was chosen (the MXCSR one is a commonly used
value).
> +static bool long_mode_active(struct x86_emulate_ctxt *ctxt)
> +{
> + uint64_t val;
> +
> + if ( fuzz_read_msr_(MSR_EFER, &val, ctxt) != X86EMUL_OKAY )
> + return false;
> +
> + return !!(val & EFER_LMA);
No need for !! here.
> +enum {
> + HOOK_read,
> + HOOK_insn_fetch,
> + HOOK_write,
> + HOOK_cmpxchg,
> + HOOK_rep_ins,
> + HOOK_rep_outs,
> + HOOK_rep_movs,
> + HOOK_rep_stos,
> + HOOK_read_segment,
> + HOOK_write_segment,
> + HOOK_read_io,
> + HOOK_write_io,
> + HOOK_read_cr,
> + HOOK_write_cr,
> + HOOK_read_dr,
> + HOOK_write_dr,
> + HOOK_read_msr,
> + HOOK_write_msr,
> + HOOK_wbinvd,
> + HOOK_cpuid,
> + HOOK_inject_hw_exception,
> + HOOK_inject_sw_interrupt,
> + HOOK_get_fpu,
> + HOOK_put_fpu,
> + HOOK_invlpg,
> + HOOK_vmfunc, /* 26 */
> + OPTION_swint_emulation = 27, /* Two bits */
> + CANONICALIZE_rip = 29,
HOOK_vmfunc,
OPTION_swint_emulation, /* Two bits */
CANONICALIZE_rip = OPTION_swint_emulation + 2,
> +/* Expects bitmap to be defined */
> +#define MAYBE_DISABLE_HOOK(h) \
> + if ( bitmap & (1 << HOOK_##h) ) \
> + { \
> + fuzz_emulops.h = NULL; \
> + printf("Disabling hook "#h"\n"); \
> + }
> +
> +static void disable_hooks(void)
> +{
> + unsigned long bitmap = input.options;
> +
> + MAYBE_DISABLE_HOOK(read);
> + MAYBE_DISABLE_HOOK(insn_fetch);
-> read and ->insn_fetch must not be NULL (there are ASSERT()s to
that effect).
> +static void set_swint_support(struct x86_emulate_ctxt *ctxt)
> +{
> + int swint_opt = (input.options >> OPTION_swint_emulation) & 3;
unsigned int
> + int map[4] = {
static const enum x86_swint_emulation
> +/*
> + * Constrain input to architecturally-possible states where
> + * the emulator relies on these
> + *
> + * In general we want the emulator to be as absolutely robust as
> + * possible; which means that we want to minimize the number of things
> + * it assumes about the input state. Tesing this means minimizing and
> + * removing as much of the input constraints as possible.
> + *
> + * So we only add constraints that (in general) have been proven to
> + * cause crashes in the emulator.
> + *
> + * For future reference: other constraints which might be necessary at
> + * some point:
> + *
> + * - EFER.LMA => !EFLAGS.NT
> + * - In VM86 mode (and real mode?), force segment...
As per commit 05118b1596 real mode should not be included here.
> + * - ...access rights to 0xf3
> + * - ...limits to 0xffff
> + * - ...bases to below 1Mb, 16-byte aligned
> + * - ...selectors to (base >> 4)
Most of this does not match the implementation (which only clear
DB for CS and SS).
> + */
> +void sanitize_input(struct x86_emulate_ctxt *ctxt) {
> + struct cpu_user_regs *regs = &input.regs;
> + unsigned long bitmap = input.options;
> +
> + input.options &=
> + ~((1<<HOOK_read)|
> + (1<<HOOK_write)|
> + (1<<HOOK_cmpxchg)|
> + (1<<HOOK_insn_fetch));
Ah, this addresses one of the earlier remarks. However, ->write
and ->cmpxchg have become optional a little while ago.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |