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

From: George Dunlap <george.dunlap@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	xen-devel@lists.xenproject.org,
	Ian Jackson <ian.jackson@citrix.com>
Subject: Re: [PATCH v3 11/12] fuzz/x86_emulate: Set and fuzz more CPU state
Date: Wed, 11 Oct 2017 17:52:27 +0100	[thread overview]
Message-ID: <5b485013-1c75-0e4d-52ed-19d1406b7d8e@citrix.com> (raw)
In-Reply-To: <59DE01180200007800184CA3@prv-mh.provo.novell.com>

On 10/11/2017 10:31 AM, Jan Beulich wrote:
>>>> On 10.10.17 at 18:20, <george.dunlap@citrix.com> 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.

<smacks head>

> 
>> @@ -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).

As I said, somewhere online I think I read that doing an fxrstor with a
certain part of the state as all zeros would "reset" the fpu.  But I
don't see that in the manual, so it's probably wrong (or at least not
architectural); in which case I should just remove the first fxrstor.

But you haven't given me clear direction about what I should do instead.
 Should I attempt to copy the functionality of fpu_fxrstor() somehow?

>> @@ -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

Ack.

 -George

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