Re: [Xen-devel] [PATCH v4 11/12] fuzz/x86_emulate: Set and fuzz more CPU state

[George Dunlap]

On 10/12/2017 04:38 PM, Jan Beulich wrote:
 On 11.10.17 at 19:52,  wrote:
>> The Intel manual claims that, "If [certain CPUID bits] are set, the
>> processor deprecates FCS and FDS, and the field is saved as h";
>> but experimentally it would be more accurate to say, "the field is
>> occasionally saved as h".  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, )
>> + */
>> +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 h".
>> + * Experimentally, however, at least on my test box,
>> + * whether this saved as h 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([0].ops, [1].ops, sizeof(state[0].ops)) )
>>  printf("ops differ!\n");
>>  
>> +if ( memcmp([0].fxsave, [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 
>> *)[0].fxsave)[i], ((unsigned *)[1].fxsave)[i]);
> 
> Long line.

Ack.

 -George

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v4 11/12] fuzz/x86_emulate: Set and fuzz more CPU state

[Jan Beulich]

>>> On 11.10.17 at 19:52,  wrote:
> The Intel manual claims that, "If [certain CPUID bits] are set, the
> processor deprecates FCS and FDS, and the field is saved as h";
> but experimentally it would be more accurate to say, "the field is
> occasionally saved as h".  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, )
> + */
> +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 h".
> + * Experimentally, however, at least on my test box,
> + * whether this saved as h 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([0].ops, [1].ops, sizeof(state[0].ops)) )
>  printf("ops differ!\n");
>  
> +if ( memcmp([0].fxsave, [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 
> *)[0].fxsave)[i], ((unsigned *)[1].fxsave)[i]);

Long line.

Jan

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v4 11/12] fuzz/x86_emulate: Set and fuzz more CPU state

[George Dunlap]

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 h";
but experimentally it would be more accurate to say, "the field is
occasionally saved as h".  This causes the --rerun checking to
trip non-deterministically.  Sanitize them to zero.

Signed-off-by: George Dunlap 
---
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 
CC: Wei Liu 
CC: Andrew Cooper 
CC: Jan Beulich 
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 102 +++-
 1 file changed, 101 insertions(+), 1 deletion(-)

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, )
+ */
+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 h".
+ * Experimentally, however, at least on my test box,
+ * whether this saved as h 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