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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.09.17 at 16:26, <george.dunlap@xxxxxxxxxx> wrote:
> @@ -597,6 +599,47 @@ static const struct x86_emulate_ops all_fuzzer_ops = {
>  };
>  #undef SET
>  
> +static void _set_fpu_state(char *fxsave, bool store)
> +{
> +    if ( cpu_has_fxsr )
> +    {
> +        static union __attribute__((__aligned__(16))) {
> +            char x[464];

The final part of the save area isn't being written, yes, but is it
really worth saving the few bytes of stack space here, rather than
having the expected 512 as array dimension?

> +            struct {
> +                uint32_t other[6];
> +                uint32_t mxcsr;
> +                uint32_t mxcsr_mask;
> +                /* ... */
> +            };
> +        } *fxs;
> +
> +        fxs = (typeof(fxs)) fxsave;
> +
> +        if ( store ) {

Style.

> +            char null[512] __attribute__((aligned(16))) = { 0 };

No need for the 0 and a blank line between declaration and statements
please.

> +            asm volatile(" fxrstor %0; "::"m"(*null));
> +            asm volatile(" fxrstor %0; "::"m"(*fxsave));

Style again - these want to follow the

    asm volatile ( "..." :: "m" (...) )

form. No need for the ; following the instructions.

> +        }
> +        
> +        asm volatile( "fxsave %0" : "=m" (*fxs) );

This is pretty confusing, the more with the different variable names
used which point to the same piece of memory. You basically store back
into the area you've read from. Is the caller expecting the memory area
to change? Is this being done other than for convenience to not have
another instance of scratch space on the stack? Some comment on the
intentions may be helpful here.

The function's parameter name being "store" adds to the confusion,
since what it controls is actually what we call "load" on x86 (or
"restore" following the insn mnemonics).

And then - what about YMM register state? Other more exotic registers
(like the BND* ones) may indeed not be that relevant to fuzz yet.

> @@ -737,6 +780,17 @@ 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 )
> +        {
> +            if ( !input_read(s, s->fxsave + (offset * 4), 4) )
> +                return;
> +            printf("Setting fxsave offset %x\n", offset * 4);

What's this 32-bit granularity derived from?

> @@ -883,6 +937,9 @@ static void sanitize_state(struct x86_emulate_ctxt *ctxt)
>          s->segments[x86_seg_cs].db = 0;
>          s->segments[x86_seg_ss].db = 0;
>      }
> +
> +    /* Setting this value seems to cause crashes in fxrstor */
> +    *((unsigned int *)(s->fxsave) + 6) = 0;

That's the MXCSR field - instead of storing zero you want to mask with
mxcsr_mask. To avoid the ugly literal 6 (and to make clear what it is
that needs adjustment here) it may also be worthwhile widening the
scope of the type declared in _set_fpu_state() and use it for struct
fuzz_state's fxsave field.

Jan


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