| Message ID | 20171011175243.19871-11-george.dunlap@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 11.10.17 at 19:52, <george.dunlap@citrix.com> wrote: > The Intel manual claims that, "If [certain CPUID bits] are set, the > processor deprecates FCS and FDS, and the field is saved as 0000h"; > but experimentally it would be more accurate to say, "the field is > occasionally saved as 0000h". This causes the --rerun checking to > trip non-deterministically. Sanitize them to zero. I think we've meanwhile settled on the field being saved as zero being a side effect of using 32-bit fxsave plus a context switch in the OS kernel. > @@ -594,6 +595,75 @@ 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 { > + uint16_t cw, sw; > + uint8_t tw, _rsvd1; > + uint16_t op; > + uint32_t ip; > + uint16_t cs, _rsvd2; > + uint32_t dp; > + uint16_t ds, _rsvd3; > + uint32_t mxcsr; > + uint32_t mxcsr_mask; > + /* ... */ > + }; > + } *fxs; > + > + fxs = (typeof(fxs))fxsave; > + > + if ( write ) > + { > + /* > + * Clear reserved bits to make sure we don't get any > + * exceptions > + */ > + fxs->mxcsr &= mxcsr_mask; > + > + /* > + * The Intel manual says that on newer models CS/DS are > + * deprecated and that these fields "are saved as 0000h". > + * Experimentally, however, at least on my test box, > + * whether this saved as 0000h or as the previously > + * written value is random; meaning that when run with > + * --rerun, we occasionally detect a "state mismatch" in these > + * bytes. Instead, simply sanitize them to zero. > + * > + * TODO Check CPUID as specified in the manual before > + * clearing > + */ > + fxs->cs = fxs->ds = 0; Shouldn't be needed anymore with ... > + asm volatile( "fxrstor %0" :: "m" (*fxs) ); rex64 (or fxrstor64) used here and ... > + } > + > + asm volatile( "fxsave %0" : "=m" (*fxs) ); ... here (of course the alternative here then is fxsave64). Also please add blanks before the opening parentheses. > @@ -732,6 +806,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 < sizeof(s->fxsave) / 4 ) You've switched to sizeof() here but ... > + { > + /* 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; ... not here. > @@ -1008,6 +1098,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++ ) Blanks around / again please. > + { > + printf("[%04lu] %08x %08x\n", I think I've indicated before that I consider leading zeros on decimal numbers misleading. Could I talk you into using %4lu instead (or really %4zu, considering the expression type) in places like this one (i.e. also in the earlier patch, where I notice only now the l -> z conversion wasn't done consistently either)? > + i * sizeof(unsigned), ((unsigned *)&state[0].fxsave)[i], ((unsigned *)&state[1].fxsave)[i]); Long line. Jan
On 10/12/2017 04:38 PM, Jan Beulich wrote: >>>> On 11.10.17 at 19:52, <george.dunlap@citrix.com> wrote: >> The Intel manual claims that, "If [certain CPUID bits] are set, the >> processor deprecates FCS and FDS, and the field is saved as 0000h"; >> but experimentally it would be more accurate to say, "the field is >> occasionally saved as 0000h". This causes the --rerun checking to >> trip non-deterministically. Sanitize them to zero. > > I think we've meanwhile settled on the field being saved as zero > being a side effect of using 32-bit fxsave plus a context switch in > the OS kernel. > >> @@ -594,6 +595,75 @@ 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 { >> + uint16_t cw, sw; >> + uint8_t tw, _rsvd1; >> + uint16_t op; >> + uint32_t ip; >> + uint16_t cs, _rsvd2; >> + uint32_t dp; >> + uint16_t ds, _rsvd3; >> + uint32_t mxcsr; >> + uint32_t mxcsr_mask; >> + /* ... */ >> + }; >> + } *fxs; >> + >> + fxs = (typeof(fxs))fxsave; >> + >> + if ( write ) >> + { >> + /* >> + * Clear reserved bits to make sure we don't get any >> + * exceptions >> + */ >> + fxs->mxcsr &= mxcsr_mask; >> + >> + /* >> + * The Intel manual says that on newer models CS/DS are >> + * deprecated and that these fields "are saved as 0000h". >> + * Experimentally, however, at least on my test box, >> + * whether this saved as 0000h or as the previously >> + * written value is random; meaning that when run with >> + * --rerun, we occasionally detect a "state mismatch" in these >> + * bytes. Instead, simply sanitize them to zero. >> + * >> + * TODO Check CPUID as specified in the manual before >> + * clearing >> + */ >> + fxs->cs = fxs->ds = 0; > > Shouldn't be needed anymore with ... > >> + asm volatile( "fxrstor %0" :: "m" (*fxs) ); > > rex64 (or fxrstor64) used here and ... > >> + } >> + >> + asm volatile( "fxsave %0" : "=m" (*fxs) ); > > ... here (of course the alternative here then is fxsave64). > > Also please add blanks before the opening parentheses. > >> @@ -732,6 +806,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 < sizeof(s->fxsave) / 4 ) > > You've switched to sizeof() here but ... > >> + { >> + /* 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; > > ... not here. > >> @@ -1008,6 +1098,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++ ) > > Blanks around / again please. > >> + { >> + printf("[%04lu] %08x %08x\n", > > I think I've indicated before that I consider leading zeros on decimal > numbers misleading. Come to think of it I agree with you. > Could I talk you into using %4lu instead (or > really %4zu, considering the expression type) in places like this one > (i.e. also in the earlier patch, where I notice only now the l -> z > conversion wasn't done consistently either)? /me looks up what %zu is supposed to do Sure. > >> + i * sizeof(unsigned), ((unsigned *)&state[0].fxsave)[i], ((unsigned *)&state[1].fxsave)[i]); > > Long line. Ack. -George
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c index f1621f98da..881c4d03c1 100644 --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c @@ -36,6 +36,7 @@ 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) @@ -594,6 +595,75 @@ 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 { + uint16_t cw, sw; + uint8_t tw, _rsvd1; + uint16_t op; + uint32_t ip; + uint16_t cs, _rsvd2; + uint32_t dp; + uint16_t ds, _rsvd3; + uint32_t mxcsr; + uint32_t mxcsr_mask; + /* ... */ + }; + } *fxs; + + fxs = (typeof(fxs))fxsave; + + if ( write ) + { + /* + * Clear reserved bits to make sure we don't get any + * exceptions + */ + fxs->mxcsr &= mxcsr_mask; + + /* + * The Intel manual says that on newer models CS/DS are + * deprecated and that these fields "are saved as 0000h". + * Experimentally, however, at least on my test box, + * whether this saved as 0000h or as the previously + * written value is random; meaning that when run with + * --rerun, we occasionally detect a "state mismatch" in these + * bytes. Instead, simply sanitize them to zero. + * + * TODO Check CPUID as specified in the manual before + * clearing + */ + fxs->cs = fxs->ds = 0; + + 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 */ @@ -669,7 +739,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) ) @@ -732,6 +806,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 < sizeof(s->fxsave) / 4 ) + { + /* 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" */ @@ -914,6 +1000,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(); @@ -925,6 +1013,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; } @@ -1008,6 +1098,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. The Intel manual claims that, "If [certain CPUID bits] are set, the processor deprecates FCS and FDS, and the field is saved as 0000h"; but experimentally it would be more accurate to say, "the field is occasionally saved as 0000h". This causes the --rerun checking to trip non-deterministically. Sanitize them to zero. Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- v4: - Remove ineffective fxrstor - Sanitize fcs and fds elements 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 | 102 +++++++++++++++++++++++- 1 file changed, 101 insertions(+), 1 deletion(-)