|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 10/12] fuzz/x86emul: update fuzzer
On Tue, Jan 31, 2017 at 06:33:11AM -0700, Jan Beulich wrote:
> >>> On 31.01.17 at 12:08, <wei.liu2@xxxxxxxxxx> wrote:
> > @@ -16,26 +17,79 @@
> >
> > #include "x86_emulate.h"
> >
> > -static unsigned char data[4096];
> > +#include "../../../xen/include/asm-x86/msr-index.h"
> > +
> > +#define MSR_INDEX_MAX 16
> > +
> > +#define SEG_NUM x86_seg_none
> > +
> > +struct input_struct {
> > + 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 input_struct, data)
> > static unsigned int data_index;
> > -static unsigned int data_max;
> > +static unsigned int data_num;
> > +
> > +/* Randomly return success or failure when processing data. If
> > + * `exception` is false, this function turns _EXCEPTION to _OKAY.
> > + */
>
> I'm not sure what coding style is to apply here, but if I consider this
> an extension to the insn emulator harness, which in turn is covered
> by hypervisor style, then the comment here needs /* to go on a
> separate line.
>
Fixed.
> > +int maybe_fail(const char *why, bool exception)
> > +{
> > + int rc;
> > +
> > + if ( data_index + 1 > data_num )
>
> "data_index >= data_num" would be the more conventional way
> to express this.
>
Fixed.
> > +static int _fuzz_rep_read(const char *why, unsigned long *reps)
> > +{
> > + int rc;
> > + unsigned long bytes_read = 0;
> > +
> > + rc = data_read(why, &bytes_read, sizeof(bytes_read));
> > +
> > + if ( bytes_read < *reps )
> > + *reps -= bytes_read;
>
> I don't understand this: In the success case, *reps upon return
> indicates the number of iterations done, not the number of
> iterations left. So at least the variable naming deserves
> improvement.
>
OK, so this should be *reps = bytes_read.
> > + /* Indicate we haven't done any work if emulation has failed */
> > + if ( rc != X86EMUL_OKAY )
> > + *reps = 0;
>
> That'll result in some code paths never taken. I'd suggest clearing
> to zero only in the UNHANDLEABLE case, and simply halving the
> amount for EXCEPTION or RETRY (granted the latter can't occur
> right now), or even using input data too for determining the new
> value.
>
No problem. This function is now like:
static int _fuzz_rep_read(const char *why, unsigned long *reps)
{
int rc;
unsigned long bytes_read = 0;
rc = data_read(why, &bytes_read, sizeof(bytes_read));
if ( bytes_read < *reps )
*reps = bytes_read;
switch ( rc )
{
case X86EMUL_UNHANDLEABLE:
*reps = 0;
break;
case X86EMUL_EXCEPTION:
case X86EMUL_RETRY:
*reps /= 2;
}
return rc;
}
And I will do the same to rep_write.
> > +static int fuzz_rep_ins(
> > + uint16_t src_port,
> > + enum x86_segment dst_seg,
> > + unsigned long dst_offset,
> > + unsigned int bytes_per_rep,
> > + unsigned long *reps,
> > + struct x86_emulate_ctxt *ctxt)
> > +{
> > + return _fuzz_rep_read("rep_in", reps);
>
> "rep_ins"
>
Fixed.
> > +static int fuzz_cpuid(
> > + uint32_t leaf,
> > + uint32_t subleaf,
> > + struct cpuid_leaf *res,
> > + struct x86_emulate_ctxt *ctxt)
> > +{
> > + return emul_test_cpuid(leaf, subleaf, res, ctxt);
> > +}
>
> I don't think you need this wrapper anymore. For the purpose of
> SET() below a simple #define will do.
>
Fixed.
> > +static int fuzz_read_segment(
> > + enum x86_segment seg,
> > + struct segment_register *reg,
> > + struct x86_emulate_ctxt *ctxt)
> > +{
> > + int rc;
> > +
> > + if ( seg > SEG_NUM )
> > + return X86EMUL_UNHANDLEABLE;
>
> As said before - this wants to be >= (even more so with
> input.segments only having SEG_NUM elements).
>
Done, and fixed write_segment too.
> > + rc = maybe_fail("read_segment", true);
> > +
> > + if ( rc == X86EMUL_OKAY )
> > + memcpy(reg, input.segments+seg, sizeof(struct segment_register));
>
> Please prefer (type safe) structure assignment.
>
> > +static int fuzz_read_cr(
> > + unsigned int reg,
> > + unsigned long *val,
> > + struct x86_emulate_ctxt *ctxt)
> > +{
> > + int rc;
> > +
> > + if ( reg > ARRAY_SIZE(input.cr) )
> > + return X86EMUL_UNHANDLEABLE;
>
> >= again.
>
Done, and fixed write_cr.
> > +enum {
> > + MSRI_IA32_SYSENTER_CS,
> > + MSRI_IA32_SYSENTER_ESP,
> > + MSRI_IA32_SYSENTER_EIP,
> > + MSRI_EFER,
> > + MSRI_STAR,
> > + MSRI_LSTAR,
> > + MSRI_CSTAR,
> > + MSRI_SYSCALL_MASK
> > +};
> > +
> > +const static unsigned int msr_index[MSR_INDEX_MAX] = {
>
> Conventionally static comes first (as not being part of the type).
>
Done.
> > +static int fuzz_read_msr(
> > + unsigned int reg,
> > + uint64_t *val,
> > + struct x86_emulate_ctxt *ctxt) {
> > + int rc;
> > +
> > + rc = maybe_fail("read_msr", true);
> > + if ( rc != X86EMUL_OKAY )
> > + return rc;
> > + else
>
> No need for "else".
>
Done.
> > +static void setup_fpu_exception_handler(void)
> > +{
> > + /* FIXME - just disable exceptions for now */
> > + unsigned long a;
> > +
> > + asm volatile ( "fnclex");
> > + a=0x37f; /* FCW_DEFAULT in Xen */
> > + asm volatile ( "fldcw %0" :: "m" (a));
> > + a=0x1f80; /* MXCSR_DEFAULT in Xen */
> > + asm volatile ( "ldmxcsr %0" :: "m" (a) );
> > +}
>
> While I see that the FCW value has changed, the strange local
> variable is still there. If you really want to keep it, please at least
> add the missing spaces around the = signs. But I'd prefer
>
> asm volatile ( "fldcw %0" :: "m" (0x37f /* FCW_DEFAULT in Xen */));
> asm volatile ( "ldmxcsr %0" :: "m" (0x1f80 /* MXCSR_DEFAULT in Xen */) );
>
This doesn't work.
x86-insn-emulator-fuzzer.c:445:5: error: memory input 0 is not directly
addressable
That's why I kept the local variable. But I will add spaces around =.
> And then - doesn't the ABI require these settings to be in effect
> upon program startup anyway?
>
I'm not sure about this -- reading AMD64 ABI Draft 0.99.8 doesn't reveal
much for me. But having the code arranged like this hasn't caused any
SIGFPE so far.
What do you suggest I do here?
> > +/*
> > + * 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 asked for before - please drop the mentioning of real mode here.
Done.
>
> > +void sanitize_input(struct x86_emulate_ctxt *ctxt) {
> > + struct cpu_user_regs *regs = &input.regs;
> > + unsigned long bitmap = input.options;
> > +
> > + /* Some hooks can't be disabled. */
> > + input.options &= ~((1<<HOOK_read)|(1<<HOOK_insn_fetch));
> > +
> > + /* Zero 'private' entries */
> > + regs->error_code = 0;
> > + regs->entry_vector = 0;
> > +
> > + CANONICALIZE_MAYBE(rip);
> > + CANONICALIZE_MAYBE(rsp);
> > + CANONICALIZE_MAYBE(rbp);
> > +
> > + /*
> > + * CR0.PG can't be set if CR0.PE isn't set. Set is more interesting,
> > so
> > + * set PE if PG is set.
> > + */
> > + if ( input.cr[0] & X86_CR0_PG )
> > + input.cr[0] |= X86_CR0_PE;
> > +
> > + /*
> > + * EFLAGS.VM not available in long mode
> > + */
> > + if ( long_mode_active(ctxt) )
> > + regs->rflags &= ~X86_EFLAGS_VM;
> > +
> > + /*
> > + * EFLAGS.VM implies 16-bit mode
> > + */
>
> Both of these are single line comments.
Done.
>
> > do {
> > + /* FIXME: Until we actually implement SIGFPE handling properly */
> > + setup_fpu_exception_handler();
>
> I don't think the comment is appropriate here - all that needs fixing is
> the body of that function (which iirc already has a respective comment).
>
Deleted this comment.
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |