| Message ID | 20171010162011.9629-11-george.dunlap@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 10.10.17 at 18:20, <george.dunlap@citrix.com> wrote: > --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c > +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c > @@ -40,6 +40,8 @@ struct fuzz_state > uint64_t msr[MSR_INDEX_MAX]; > struct segment_register segments[SEG_NUM]; > struct cpu_user_regs regs; > + char fxsave[512] __attribute__((aligned(16))); > + > > /* Fuzzer's input data. */ No double blank lines please. > @@ -596,6 +598,54 @@ static const struct x86_emulate_ops all_fuzzer_ops = { > }; > #undef SET > > +/* > + * This funciton will read or write fxsave to the fpu. When writing, > + * it 'sanitizes' the state: It will mask off the appropriate bits in > + * the mxcsr, 'restore' the state to the fpu, then 'save' it again so > + * that the data in fxsave reflects what's actually in the FPU. > + * > + * TODO: Extend state beyond just FPU (ymm registers, &c) > + */ > +static void _set_fpu_state(char *fxsave, bool write) > +{ > + if ( cpu_has_fxsr ) > + { > + static union __attribute__((__aligned__(16))) { > + char x[512]; > + struct { > + uint32_t other[6]; > + uint32_t mxcsr; > + uint32_t mxcsr_mask; > + /* ... */ > + }; > + } *fxs; > + > + fxs = (typeof(fxs)) fxsave; Stray blank after the cast. > + if ( write ) > + { > + char null[512] __attribute__((aligned(16))) = { }; > + > + fxs->mxcsr &= mxcsr_mask; > + > + asm volatile( "fxrstor %0" :: "m" (*null) ); > + asm volatile( "fxrstor %0" :: "m" (*fxs) ); Without a comment I still don't really understand what good this double load is intended to do. In particular I don't think there are any state components that the first may alter but the second won't. Quite the opposite, on AMD I think you may end up not altering FIP/FDP/FOP despite this double load (iirc they're being loaded only when an exception is indicated to be pending in the status word; see fpu_fxrstor() in the hypervisor). > @@ -737,6 +791,18 @@ static void setup_state(struct x86_emulate_ctxt *ctxt) > printf("Setting cpu_user_regs offset %x\n", offset); > continue; > } > + offset -= sizeof(struct cpu_user_regs); > + > + /* Fuzz fxsave state */ > + if ( offset < 128 ) sizeof(s->fxsave) / 4 Jan
On 10/11/2017 10:31 AM, Jan Beulich wrote: >>>> On 10.10.17 at 18:20, <george.dunlap@citrix.com> wrote: >> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c >> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c >> @@ -40,6 +40,8 @@ struct fuzz_state >> uint64_t msr[MSR_INDEX_MAX]; >> struct segment_register segments[SEG_NUM]; >> struct cpu_user_regs regs; >> + char fxsave[512] __attribute__((aligned(16))); >> + >> >> /* Fuzzer's input data. */ > > No double blank lines please. <smacks head> > >> @@ -596,6 +598,54 @@ static const struct x86_emulate_ops all_fuzzer_ops = { >> }; >> #undef SET >> >> +/* >> + * This funciton will read or write fxsave to the fpu. When writing, >> + * it 'sanitizes' the state: It will mask off the appropriate bits in >> + * the mxcsr, 'restore' the state to the fpu, then 'save' it again so >> + * that the data in fxsave reflects what's actually in the FPU. >> + * >> + * TODO: Extend state beyond just FPU (ymm registers, &c) >> + */ >> +static void _set_fpu_state(char *fxsave, bool write) >> +{ >> + if ( cpu_has_fxsr ) >> + { >> + static union __attribute__((__aligned__(16))) { >> + char x[512]; >> + struct { >> + uint32_t other[6]; >> + uint32_t mxcsr; >> + uint32_t mxcsr_mask; >> + /* ... */ >> + }; >> + } *fxs; >> + >> + fxs = (typeof(fxs)) fxsave; > > Stray blank after the cast. > >> + if ( write ) >> + { >> + char null[512] __attribute__((aligned(16))) = { }; >> + >> + fxs->mxcsr &= mxcsr_mask; >> + >> + asm volatile( "fxrstor %0" :: "m" (*null) ); >> + asm volatile( "fxrstor %0" :: "m" (*fxs) ); > > Without a comment I still don't really understand what good this > double load is intended to do. In particular I don't think there are > any state components that the first may alter but the second > won't. Quite the opposite, on AMD I think you may end up not > altering FIP/FDP/FOP despite this double load (iirc they're being > loaded only when an exception is indicated to be pending in the > status word; see fpu_fxrstor() in the hypervisor). As I said, somewhere online I think I read that doing an fxrstor with a certain part of the state as all zeros would "reset" the fpu. But I don't see that in the manual, so it's probably wrong (or at least not architectural); in which case I should just remove the first fxrstor. But you haven't given me clear direction about what I should do instead. Should I attempt to copy the functionality of fpu_fxrstor() somehow? >> @@ -737,6 +791,18 @@ static void setup_state(struct x86_emulate_ctxt *ctxt) >> printf("Setting cpu_user_regs offset %x\n", offset); >> continue; >> } >> + offset -= sizeof(struct cpu_user_regs); >> + >> + /* Fuzz fxsave state */ >> + if ( offset < 128 ) > > sizeof(s->fxsave) / 4 Ack. -George
>>> On 11.10.17 at 18:52, <george.dunlap@citrix.com> wrote: > On 10/11/2017 10:31 AM, Jan Beulich wrote: >>>>> On 10.10.17 at 18:20, <george.dunlap@citrix.com> wrote: >>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c >>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c >>> @@ -40,6 +40,8 @@ struct fuzz_state >>> uint64_t msr[MSR_INDEX_MAX]; >>> struct segment_register segments[SEG_NUM]; >>> struct cpu_user_regs regs; >>> + char fxsave[512] __attribute__((aligned(16))); >>> + >>> >>> /* Fuzzer's input data. */ >> >> No double blank lines please. > > <smacks head> Please don't. >>> + if ( write ) >>> + { >>> + char null[512] __attribute__((aligned(16))) = { }; >>> + >>> + fxs->mxcsr &= mxcsr_mask; >>> + >>> + asm volatile( "fxrstor %0" :: "m" (*null) ); >>> + asm volatile( "fxrstor %0" :: "m" (*fxs) ); >> >> Without a comment I still don't really understand what good this >> double load is intended to do. In particular I don't think there are >> any state components that the first may alter but the second >> won't. Quite the opposite, on AMD I think you may end up not >> altering FIP/FDP/FOP despite this double load (iirc they're being >> loaded only when an exception is indicated to be pending in the >> status word; see fpu_fxrstor() in the hypervisor). > > As I said, somewhere online I think I read that doing an fxrstor with a > certain part of the state as all zeros would "reset" the fpu. But I > don't see that in the manual, so it's probably wrong (or at least not > architectural); in which case I should just remove the first fxrstor. > > But you haven't given me clear direction about what I should do instead. > Should I attempt to copy the functionality of fpu_fxrstor() somehow? Well, what (if anything) to do about this depends on what you want to achieve. Random input to FXRSTOR isn't going to be a problem if you don't expect said fields to never change across emulation. FPU insns may result in the fields changing in any event. If the fields appearing to have changed without any FPU insn having been emulated is not a problem, not doing anything may be good enough. Otherwise cloning some of the logic from fpu_fxrstor() may be necessary, but to give more precise advice I'd prefer to know some more detail on what unreliable state transition is seen with what insn sequence. Jan
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c index 7685e976b8..79dd36ec30 100644 --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c @@ -40,6 +40,8 @@ struct fuzz_state uint64_t msr[MSR_INDEX_MAX]; struct segment_register segments[SEG_NUM]; struct cpu_user_regs regs; + char fxsave[512] __attribute__((aligned(16))); + /* Fuzzer's input data. */ #define DATA_SIZE_FULL offsetof(struct fuzz_state, corpus) @@ -596,6 +598,54 @@ static const struct x86_emulate_ops all_fuzzer_ops = { }; #undef SET +/* + * This funciton will read or write fxsave to the fpu. When writing, + * it 'sanitizes' the state: It will mask off the appropriate bits in + * the mxcsr, 'restore' the state to the fpu, then 'save' it again so + * that the data in fxsave reflects what's actually in the FPU. + * + * TODO: Extend state beyond just FPU (ymm registers, &c) + */ +static void _set_fpu_state(char *fxsave, bool write) +{ + if ( cpu_has_fxsr ) + { + static union __attribute__((__aligned__(16))) { + char x[512]; + struct { + uint32_t other[6]; + uint32_t mxcsr; + uint32_t mxcsr_mask; + /* ... */ + }; + } *fxs; + + fxs = (typeof(fxs)) fxsave; + + if ( write ) + { + char null[512] __attribute__((aligned(16))) = { }; + + fxs->mxcsr &= mxcsr_mask; + + asm volatile( "fxrstor %0" :: "m" (*null) ); + asm volatile( "fxrstor %0" :: "m" (*fxs) ); + } + + asm volatile( "fxsave %0" : "=m" (*fxs) ); + } +} + +static void set_fpu_state(char *fxsave) +{ + _set_fpu_state(fxsave, true); +} + +static void save_fpu_state(char *fxsave) +{ + _set_fpu_state(fxsave, false); +} + static void setup_fpu_exception_handler(void) { /* FIXME - just disable exceptions for now */ @@ -674,7 +724,11 @@ static void setup_state(struct x86_emulate_ctxt *ctxt) return; } - /* Modify only select bits of state */ + /* + * Modify only select bits of state. In general, try not to fuzz less + * than 32 bits at a time; otherwise we're reading 2 bytes in order to fuzz only + * one byte. + */ /* Always read 'options' */ if ( !input_read(s, s, DATA_SIZE_COMPACT) ) @@ -737,6 +791,18 @@ static void setup_state(struct x86_emulate_ctxt *ctxt) printf("Setting cpu_user_regs offset %x\n", offset); continue; } + offset -= sizeof(struct cpu_user_regs); + + /* Fuzz fxsave state */ + if ( offset < 128 ) + { + /* 32-bit size is arbitrary; see comment above */ + if ( !input_read(s, s->fxsave + (offset * 4), 4) ) + return; + printf("Setting fxsave offset %x\n", offset * 4); + continue; + } + offset -= 128; /* None of the above -- take that as "start emulating" */ @@ -919,6 +985,8 @@ static int runtest(struct fuzz_state *state) { disable_hooks(state); + set_fpu_state(state->fxsave); + do { /* FIXME: Until we actually implement SIGFPE handling properly */ setup_fpu_exception_handler(); @@ -930,6 +998,8 @@ static int runtest(struct fuzz_state *state) { printf("Emulation result: %d\n", rc); } while ( rc == X86EMUL_OKAY ); + save_fpu_state(state->fxsave); + return 0; } @@ -1013,6 +1083,16 @@ static void compare_states(struct fuzz_state state[2]) if ( memcmp(&state[0].ops, &state[1].ops, sizeof(state[0].ops)) ) printf("ops differ!\n"); + if ( memcmp(&state[0].fxsave, &state[1].fxsave, sizeof(state[0].fxsave)) ) + { + printf("fxsave differs!\n"); + for ( i = 0; i < sizeof(state[0].fxsave)/sizeof(unsigned); i++ ) + { + printf("[%04lu] %08x %08x\n", + i * sizeof(unsigned), ((unsigned *)&state[0].fxsave)[i], ((unsigned *)&state[1].fxsave)[i]); + } + } + if ( memcmp(&state[0].ctxt, &state[1].ctxt, sizeof(state[0].ctxt)) ) { printf("ctxt differs!\n");
x86_emulate() operates not only on state passed to it in cpu_user_regs, but also on state currently found on the cpu: namely, the FPU and XMM registers. At the moment, we re-zero (and/or re-initialize) cpu_user_regs on every invocation, but leave the cpu-stored state alone. In "persistent mode", this causes test cases to behave differently -- sometimes significantly so -- depending on which test cases have been run beforehand. Zero out the state before each test run, and then fuzz it based on the corpus input. Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- v3: - Make type 512 bytes rather than 464 - Style changes - Change argument from 'store' to 'write' - Add a comment explaining why we always 'save' even for a write - Sanitize mxcsr with mxcrs_mask when writing instead of zeroing it in sanitize_state - Get rid of redundant mxcsr_mask setting - Add comments explaining why we're arbitrarily writing 32 bits v2: Rebase on top of previous changes CC: Ian Jackson <ian.jackson@citrix.com> CC: Wei Liu <wei.liu2@citrix.com> CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> --- tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 82 ++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-)