Re: [Xen-devel] [PATCH v2 13/19] x86/shadow: Avoid raising faults behind the emulators back

[Tim Deegan <tim@xxxxxxx>]

At 16:04 +0000 on 28 Nov (1480349059), Andrew Cooper wrote:
> On 28/11/16 14:49, Tim Deegan wrote:
> > At 11:13 +0000 on 28 Nov (1480331610), Andrew Cooper wrote:
> >> +        /*
> >> +         * This emulation covers writes to shadow pagetables.  We 
> >> tolerate #PF
> >> +         * (from hitting adjacent pages), #GP/#SS (from segmentation 
> >> errors),
> >> +         * and #DB (from singlestepping).  Anything else is an emulation 
> >> bug,
> >> +         * or a guest playing with the instruction stream under Xen's 
> >> feet.
> >> +         */
> >> +        if ( emul_ctxt.ctxt.event.type == X86_EVENTTYPE_HW_EXCEPTION &&
> >> +             (emul_ctxt.ctxt.event.vector < 32) &&
> >> +             ((1u << emul_ctxt.ctxt.event.vector) &
> >> +              ((1u << TRAP_debug) | (1u << TRAP_stack_error) |
> >> +               (1u << TRAP_gp_fault) | (1u << TRAP_page_fault))) )
> >> +        {
> >> +            if ( is_hvm_vcpu(v) )
> >> +                hvm_inject_event(&emul_ctxt.ctxt.event);
> >> +            else
> >> +                pv_inject_event(&emul_ctxt.ctxt.event);
> >> +        }
> >> +        else
> >> +        {
> >> +            if ( is_hvm_vcpu(v) )
> >> +                hvm_inject_hw_exception(TRAP_gp_fault, 0);
> >> +            else
> >> +                pv_inject_hw_exception(TRAP_gp_fault, 0);
> >> +        }
> > I don't think it's OK to lob #GP into a HVM guest here -- we can hit
> > this path even if the guest isn't behaving strangely.
> 
> What circumstances are you thinking of?
> 
> Unless the guest is playing with the instruction stream,  event_pending
> will only be set by the set in the paths altered by this patch, which is
> the four whitelisted vectors.

I don't see any path through the emulator that both writes to memory
and raises a different exception to these four, so I retract that -
the guest does have to be acting strangely.  Likely either modifying
code while another CPU is running on it or executing from a page whose
PT mappings are changing (including changing a text mapping and then
executing from it without flushing TLB first).

But even if the guest is doing something demented with multithreaded
self-modifying code, I think we're better off abandoning ship here
and retrying the instruction than raising a spurious #GP.  HVM
guests don't have any contract with us that lets us #GP them here.
And we can hit this path for writes to non-pagetables, if we haven't
unshadowed an ex-pagetable yet.

I don't know of any kernel-space code that behaves quite so loosely
wiht its mappings, but obfuscation tricks to stop reverse engineering
can do some pretty strange things.

> > I think it would be better to run this check before the 
> > X86EMUL_UNHANDLEABLE one
> > and convert injections that we choose not to handle into
> > X86EMUL_UNHANDLEABLE.
> >
> > Which I guess brings us back full circle to the behaviour we had
> > before, and perhaps the right change to the earlier patch is to
> > start down this road, with an explanatory comment.
> >
> > e.g. start with something like this, immediately after the first call
> > to x86_emulate():
> >
> >     /*
> >      * Events raised within the emulator itself used to return
> >      * X86EMUL_UNHANDLEABLE because we didn't supply injection
> >      * callbacks.  Now the emulator supplies those to us via
> >      * ctxt.event_pending instead.  Preserve the old behaviour
> >      * for now.
> >      */
> >      if (emul_ctxt.ctxt.event_pending)
> >          r = X86EMUL_UNHANDLEABLE;
> >
> > And in this patch, replace it with the hunk you have above, but
> > setting r = X86EMUL_UNHANDLEABLE instead of injecting #GP.
> >
> > Does that make sense?
> 
> It does make sense, but that goes against the comment of not unshadowing
> on exception.

Yep, that comment would need to be updated.  The final behaviour is
something like what we had before: some kinds of event (#PF in
particular) get injected, and for the others we unshadow and retry.
You've expanded the set of events that we'll inject, and the mechanism has
moved around.

> A lot of this revolves around how likely we are to hit the #GP[0] case. 
> I assert that we shouldn't be able to hit it unless the guest is playing
> games, or we have a bug in emulation.
> 
> If we don't go throwing a #GP back, we should at least leave something
> obvious in the log.
> 
> >
> > Also, I'm a little confused after all this as to whether the emulator
> > can still return with X86EMUL_OKAY and the event_pending set.
> 
> I have now determined this not to be the case.  Apologies for my
> misinformation in v1.

Phew!

> > This looks like code duplication, but rather than trying to merge the
> > two cases, I think we can drop this one entirely.  This emulation is
> > optimistically trying to find the second half of a PAE PTE write -
> > it's OK just to stop emulating if we hit anything this exciting.
> > So we can lose the whole hunk.
> 
> At the very least we should retain the singlestep #DB injection, as it
> still has trap semantics.

Argh!  Meaning it returns X86EMUL_EXCEPTION but has already updated
register state?  So yeah, we have to inject that.  But it can go away
when you change everything to have fault semantics, right?

Tim.

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