Re: [PATCH] x86emul: avoid triggering event related assertions

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/04/2023 3:15 pm, Jan Beulich wrote:
> The assertion at the end of x86_emulate_wrapper() as well as the ones
> in x86_emul_{hw_exception,pagefault}() can trigger if we ignore
> X86EMUL_EXCEPTION coming back from certain hook functions.

Looking at the comment I wrote back then, I don't think I'd considered
this case.

What I was fixing at the time was the case where
hvm_inject_hw_exception() had raised an exception behind the back of the
emulator, and any subsequent exception escalates to #DF.

I guess the comment wants updating to discuss this problem too, where
the hook queued an exception but we didn't squash it properly when
deciding to ignore it.

As it's debugging-only anyway, it might be worth rearranging the
expression to be

if ( ctxt->event_pending )
    ASSERT(rc == X86EMUL_EXCEPTION);
else
    ASSERT(rc != X86EMUL_EXCEPTION);

because it distinguishes the two cases without an intermediate round of
debugging.

>  Squash
> exceptions when merely probing MSRs, plus on SWAPGS'es "best effort"
> error handling path.
>
> In adjust_bnd() add another assertion after the read_xcr(0, ...)
> invocation, paralleling the one in x86emul_get_fpu() - XCR0 reads should
> never fault when XSAVE is (implicitly) known to be available.
>
> Fixes: 14a6be89ec04 ("x86emul: correct EFLAGS.TF handling")
> Fixes: cb2626c75813 ("x86emul: conditionally clear BNDn for branches")
> Fixes: 6eb43fcf8a0b ("x86emul: support SWAPGS")
> Reported-by: AFL
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> EFER reads won't fault in any of the handlers we have, so in principle
> the respective check could be omitted (and hence has no Fixes: tag).
> Thoughts?

We already require LMA as input state, and don't have an execution model
where EFER is actually absent in the first place, so passing the whole
thing wouldn't really be an issue.

I have previously considered doing the same for CR0 too, but at best
it's marginal so I haven't got around to trying it.

> --- a/xen/arch/x86/x86_emulate/0fae.c
> +++ b/xen/arch/x86/x86_emulate/0fae.c
> @@ -67,7 +67,10 @@ int x86emul_0fae(struct x86_emulate_stat
>                      cr4 = X86_CR4_OSFXSR;
>                  if ( !ops->read_msr ||
>                       ops->read_msr(MSR_EFER, &msr_val, ctxt) != X86EMUL_OKAY 
> )
> +                {
> +                    x86_emul_reset_event(ctxt);

This is the only path you've introduced that doesn't restrict the reset
to the X86EMUL_EXCEPTION case.

In principle you can get things like RETRY for introspection. 
Internally, UNHANDLEABLE is used but I hope that never escapes from
guest_{rd,wr}msr().

~Andrew