Re: [Xen-devel] [PATCH v2] x86emul/fuzz: add a state sanitization function

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Apr 1, 2019, at 8:46 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> 
> This is to accompany sanitize_input(). Just like for initial state we
> want to have state between two emulated insns sane, at least as far as
> assumptions in the main emulator go. Do minimal checking after segment
> register, CR, and MSR writes, and roll back to the old value in case of
> failure (raising #GP(0) at the same time).
> 
> In the particular case observed, a CR0 write clearing CR0.PE was
> followed by a VEX-encoded insn, which the decoder accepts based on
> guest address size, restricting things just outside of the 64-bit case
> (real and virtual modes don't allow VEX-encoded insns). Subsequently
> _get_fpu() would then assert that CR0.PE must be set (and EFLAGS.VM
> clear) when trying to invoke YMM, ZMM, or OPMASK state.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

> ---
> v2: Correct placement of new declaration in fuzz_write_segment().
> 
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -76,6 +76,8 @@ static inline bool input_read(struct fuz
>     return true;
> }
> 
> +static bool sanitize_state(struct x86_emulate_ctxt *ctxt);
> +
> static const char* const x86emul_return_string[] = {
>     [X86EMUL_OKAY] = "X86EMUL_OKAY",
>     [X86EMUL_UNHANDLEABLE] = "X86EMUL_UNHANDLEABLE",
> @@ -424,8 +426,19 @@ static int fuzz_write_segment(
>     rc = maybe_fail(ctxt, "write_segment", true);
> 
>     if ( rc == X86EMUL_OKAY )
> +    {
> +        struct segment_register old = c->segments[seg];
> +
>         c->segments[seg] = *reg;
> 
> +        if ( !sanitize_state(ctxt) )
> +        {
> +            c->segments[seg] = old;
> +            x86_emul_hw_exception(13 /* #GP */, 0, ctxt);
> +            rc = X86EMUL_EXCEPTION;
> +        }
> +    }
> +
>     return rc;
> }
> 
> @@ -452,6 +465,7 @@ static int fuzz_write_cr(
> {
>     struct fuzz_state *s = ctxt->data;
>     struct fuzz_corpus *c = s->corpus;
> +    unsigned long old;
>     int rc;
> 
>     if ( reg >= ARRAY_SIZE(c->cr) )
> @@ -461,9 +475,17 @@ static int fuzz_write_cr(
>     if ( rc != X86EMUL_OKAY )
>         return rc;
> 
> +    old = c->cr[reg];
>     c->cr[reg] = val;
> 
> -    return X86EMUL_OKAY;
> +    if ( !sanitize_state(ctxt) )
> +    {
> +        c->cr[reg] = old;
> +        x86_emul_hw_exception(13 /* #GP */, 0, ctxt);
> +        rc = X86EMUL_EXCEPTION;
> +    }
> +
> +    return rc;
> }
> 
> #define fuzz_read_xcr emul_test_read_xcr
> @@ -561,7 +583,16 @@ static int fuzz_write_msr(
>     {
>         if ( msr_index[idx] == reg )
>         {
> +            uint64_t old = c->msr[idx];
> +
>             c->msr[idx] = val;
> +
> +            if ( !sanitize_state(ctxt) )
> +            {
> +                c->msr[idx] = old;
> +                break;
> +            }
> +
>             return X86EMUL_OKAY;
>         }
>     }
> @@ -808,6 +839,30 @@ static void sanitize_input(struct x86_em
>     }
> }
> 
> +/*
> + * Call this function from hooks potentially altering machine state into
> + * something that's not architecturally valid, yet which - as per above -
> + * the emulator relies on.
> + */
> +static bool sanitize_state(struct x86_emulate_ctxt *ctxt)
> +{
> +    const struct fuzz_state *s = ctxt->data;
> +    const struct fuzz_corpus *c = s->corpus;
> +    const struct cpu_user_regs *regs = &c->regs;
> +
> +    if ( long_mode_active(ctxt) && !(c->cr[0] & X86_CR0_PG) )
> +        return false;
> +
> +    if ( (c->cr[0] & X86_CR0_PG) && !(c->cr[0] & X86_CR0_PE) )
> +        return false;
> +
> +    if ( (regs->rflags & X86_EFLAGS_VM) &&
> +         (c->segments[x86_seg_cs].db || c->segments[x86_seg_ss].db) )
> +        return false;
> +
> +    return true;
> +}

Sorry, I didn’t read this function very well on Friday.  It’s not actually 
doing any sanitation; rather, it’s checking whether the state is 
architecturally valid.  Or more precisely: it’s checking whether the emulator's 
assumptions about the state still hold.

check_state?  sanity_check_state?  

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel