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
