Re: [Xen-devel] [PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/09/17 15:26, George Dunlap wrote:
> From: Jan Beulich <jbeulich@xxxxxxxx>
>
> fuzz_insn_fetch() is the only data access helper where it is possible
> to see offsets larger than 4Gb in 16- or 32-bit modes, as we leave the
> incoming rIP untouched in the emulator itself.

Is it reasonable to tolerate this?  AFAICT, this is only because we have
unsanitised fuzzing data in the input?

VMX will fail a vmentry if any of the upper 32 %rip bits are set if CS.L
is clear.  (OTOH, the next check in the list is the canonical check,
which is bogus on vmentry, so maybe this isn't a good example to take.)

>  The check is needed here
> as otherwise, after successfully fetching insn bytes, we may end up
> zero-extending EIP soon after complete_insn, which collides with the
> X86EMUL_EXCEPTION-conditional respective ASSERT() in
> x86_emulate_wrapper().

In light of this, even more reason that the wrapper should gain:

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c
b/xen/arch/x86/x86_emulate/x86_emulate.c
index a68676c..4845cad 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -7944,6 +7944,8 @@ int x86_emulate_wrapper(
 
     if ( mode_64bit() )
         ASSERT(ctxt->lma);
+    else
+        ASSERT((orig_ip >> 32) == 0);
 
     rc = x86_emulate(ctxt, ops);
 


>  (NB: put_rep_prefix() is what allows
> complete_insn to be reached with rc set to other than X86EMUL_OKAY or
> X86EMUL_DONE. See also commit 53f87c03b4 ["x86emul: generalize
> exception handling for rep_* hooks"].)
>
> Add assert()-s for all other (data) access routines, as effective
> address generation in the emulator ought to guarantee in-range values.
> For them to not trigger, several adjustments to the emulator's address
> calculations are needed: While for DstBitBase it is really mandatory,
> the specification allows for either behavior for two-part accesses.

I can't parse this sentence.  What does the "it" following DstBitBase
refer to, and which two things do "either behaviours" refer to?

> Observed behavior on real hardware, however, is for such accesses to
> silently wrap at the 2^^32 boundary in other than 64-bit mode, just
> like they do at the 2^^64 boundary in 64-bit mode. While adding
> truncate_ea() invocations there, also convert open coded instances of
> it.
>
> Reported-by: George Dunlap <george.dunlap@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> CC: Ian Jackson <ian.jackson@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> CC: Jan Beulich <jbeulich@xxxxxxxx>
> ---
>  tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 32 
> ++++++++++++++++++++++---
>  xen/arch/x86/x86_emulate/x86_emulate.c          | 22 +++++++++--------
>  2 files changed, 41 insertions(+), 13 deletions(-)
>
> diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c 
> b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> index a2329f84a5..105145e9f9 100644
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -139,7 +139,18 @@ static int fuzz_read(
>      struct x86_emulate_ctxt *ctxt)
>  {
>      /* Reads expected for all user and system segments. */
> -    assert(is_x86_user_segment(seg) || is_x86_system_segment(seg));
> +    if ( is_x86_user_segment(seg) )
> +        assert(ctxt->addr_size == 64 || !(offset >> 32));
> +    else if ( seg == x86_seg_tr )
> +        /*
> +         * The TSS is special in that accesses below the segment base are
> +         * possible, as the Interrupt Redirection Bitmap starts 32 bytes
> +         * ahead of the I/O Bitmap, regardless of the value of the latter.
> +         */
> +        assert((long)offset < 0 ? (long)offset > -32 : !(offset >> 17));
> +    else
> +        assert(is_x86_system_segment(seg) &&
> +               (ctxt->lma ? offset <= 0x10007 : !(offset >> 16)));

Where do the extra 7 bytes come from in long mode?  I'm not aware of an
architectural way to make misaligned offsets into the LDT/GDT/IDT.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel