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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.10.17 at 19:52, <george.dunlap@xxxxxxxxxx> 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

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