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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.10.17 at 14:54, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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?

Yes, that's what I recall, i.e.

> 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.)

... this has nothing to do with the former. The new %rip will
always be clipped.

>>  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);

I'm not convinced - the emulator works fine without this check, and
I don't think we should assert on inputs coming from various sources.
If anything I could see us return UNHANDLEABLE in that case.

>>  (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?

See the discussion we've have with George. The replacement text
now reads

"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 the DstBitBase one is really mandatory,
 the specification allows for either original or new behavior for two-
 part accesses. 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, which our
 code is now being brought in line with. While adding truncate_ea()
 invocations there, also convert open coded instances of it."

>> --- 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.

It's 8 extra bytes - when we read a gate descriptor, the upper
half may exceed the standard 64k limit. This is part of why the
title says "rudimentary".

Jan

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