Re: Assert in x86_emulate_wrapper triggerable by HVM domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.04.2025 23:52, Manuel Andreas wrote:
> 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.
> 
> 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:

If my analysis is correct, none of this matters here; the core emulator
is working correctly. Hence also why the in-tree fuzzer wouldn't have
caught it. Would you please give the patch a try that I just sent, with
Cc to you (sorry, the list archive didn't pick it up yet, hence no link)?

Jan

>     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  })
> 
>    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
> 
> Best,
> Manuel