[Xen-devel] [PATCH] x86emul: refine VME/PVI early #GP check

[Jan Beulich]

In commit efe9cba66c ("x86emul: VME and PVI modes require a #GP(0) check
first thing") I neglected the fact that the retire flags get zapped only
in x86_decode(), which hasn't been invoked yet at the point of the #GP(0)
check added.

Ahead of the other explicit return (rather than "goto") path use an
explicit return here too, to make the flow more obvious.

Signed-off-by: Jan Beulich <***@suse.com>

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -3400,10 +3400,15 @@ x86_emulate(

ASSERT(ops->read);

- generate_exception_if((mode_vif() &&
- (_regs.eflags & X86_EFLAGS_VIF) &&
- (_regs.eflags & X86_EFLAGS_VIP)),
- EXC_GP, 0);
+ if ( unlikely(mode_vif()) &&
+ (_regs.eflags & X86_EFLAGS_VIF) &&
+ (_regs.eflags & X86_EFLAGS_VIP) )
+ {
+ x86_emul_hw_exception(EXC_GP, 0, ctxt);
+ /* x86_decode() was not called yet. */
+ ctxt->retire.raw = 0;
+ return X86EMUL_EXCEPTION;
+ }

rc = x86_decode(&state, ctxt, ops);
if ( rc != X86EMUL_OKAY )

[Jan Beulich]

Post by Jan Beulich
In commit efe9cba66c ("x86emul: VME and PVI modes require a #GP(0) check
first thing") I neglected the fact that the retire flags get zapped only
in x86_decode(), which hasn't been invoked yet at the point of the #GP(0)
check added.
Ahead of the other explicit return (rather than "goto") path use an
explicit return here too, to make the flow more obvious.

How did you discover this issue?

AFL discovered it for me.

I ask because given the comment, its
quite likely that you'll hit the assert in x86_emul_hw_exception()

Hmm, good point: I'll need to call x86_emul_reset_event()
as well.

Why is the zeroing code in decode rather than emulate?

Because x86_decode() has another caller. Best I can suggest
is that we make x86_emulate() and x86_decode_insn() call a
shared helper function doing the setup.

I'm tempted to
suggest that we fix this by requiring that callers pass in a suitably
initialised ctxt.

We've discussed requiring callers to set state, but I continue to
think that it makes no sense to require this for output-only state.
Furthermore what would callers set it to? Zeroing everything
would already imply knowledge of the internal workings.

Jan