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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.10.17 at 18:20, <george.dunlap@xxxxxxxxxx> 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


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