Re: Assert in x86_emulate_wrapper triggerable by HVM domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2025 23:52, Manuel Andreas wrote:
> Dear all,
> 
> my fuzzing infrastructure discovered that an assert in 
> x86_emulate_wrapper is able to be triggered by an HVM domain executing a 
> specially crafted repeating movs instruction.

That's

    /* All cases returning X86EMUL_EXCEPTION should have fault semantics. */
    if ( rc == X86EMUL_EXCEPTION )
        ASSERT(ctxt->regs->r(ip) == orig_ip);

?

> Specifically, if the emulation of the rep movs instruction triggers an 
> exception (e.g. by accessing invalid memory after some amount of 
> iterations), the emulation will be halted at that point.
> However, the instruction manual requires that _some_ register state 
> (namely the updated value of rcx) shall be commited, whereas the 
> instruction pointer needs to be rolled back to point to the address of 
> the instruction itself. The assert checks for the latter. Problematic is 
> the fact that for these type of repeating instructions, Xen seems to 
> eventually just commit all register state when it encounters an exception:
> 
>     557  #define put_rep_prefix(reps_completed) 
> ({                               \
>     558      if ( rep_prefix() 
> )                                                 \
>     559 { \
>     560          __put_rep_prefix(&_regs, ctxt->regs, ad_bytes, 
> reps_completed); \
>     561          if ( unlikely(rc == X86EMUL_EXCEPTION) 
> )                        \
>     562              goto 
> complete_insn;                                         \
>     563 } \
>     564  })

I fear I'm not following, considering __put_rep_prefix() has

    /* Reduce counter appropriately, and repeat instruction if non-zero. */
    ecx -= reps_completed;
    if ( ecx != 0 )
        int_regs->r(ip) = ext_regs->r(ip);

>    8356   complete_insn: /* Commit shadow register state. */
>    8357      put_fpu(fpu_type, false, state, ctxt, ops);
>    8358      fpu_type = X86EMUL_FPU_none;
>    8359
>    8360      /* Zero the upper 32 bits of %rip if not in 64-bit mode. */
>    8361      if ( !mode_64bit() )
>    8362          _regs.r(ip) = (uint32_t)_regs.r(ip);
>    8363
>    8364      /* Should a singlestep #DB be raised? */
>    8365      if ( rc == X86EMUL_OKAY && singlestep && !ctxt->retire.mov_ss )
>    8366      {
>    8367          ctxt->retire.singlestep = true;
>    8368          ctxt->retire.sti = false;
>    8369      }
>    8370
>    8371      if ( rc != X86EMUL_DONE )
>    8372          *ctxt->regs = _regs; // <- Incorrect RIP is commited
> 
> I've attached an XTF test that should trigger the aforementioned assert 
> on the latest release commit: 3ad5d648cda5add395f49fc3704b2552aae734f7

I'll give this a try, perhaps it'll shed more light on the situation. I'd
be surprised though that the fuzzer we have in tree wouldn't have found
this (so far).

Jan