Re: [PATCH v3] x86emul/fuzz: add rudimentary limit checking

[George Dunlap <george.dunlap@citrix.com>]

On 07/06/2017 04:21 PM, Jan Beulich wrote:
>>>> On 06.07.17 at 16:02, <george.dunlap@citrix.com> wrote:
>> On 07/06/2017 01:34 PM, Jan Beulich wrote:
>>>>>> On 06.07.17 at 12:57, <george.dunlap@citrix.com> wrote:
>>>> On 07/06/2017 10:20 AM, Jan Beulich wrote:
>>>>> 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. 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(). (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.
>>>>> 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@citrix.com>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> ---
>>>>> v3: Add more truncate_ea().
>>>>> v2: Correct system segment related assert()-s.
>>>>
>>>> Still getting crashes in protmode_load_seg(), line 1824.  (See attached
>>>> for an example stack trace; but basically any place that calls
>>>> protmode_load_seg()).
>>>
>>> Ah, this is one I indeed forgot about. We shouldn't deal with this in
>>> the emulator though, so slightly relaxing the assert() seems like the
>>> only option: We'd need to permit reads up to 0x10007 instead of
>>> 0xffff (which would never pass limit checks).
>>
>> Replacing !(offset >> 16) with (offset <= 0x10007) makes all the current
>> crash cases I have pass.
>>
>> If you want I can submit this patch, modified, with my series of afl
>> fixes / changes.
> 
> I've done the above change slightly differently (distinguishing long
> from legacy modes), so if you want to put it in your series, please
> use the attached variant (aka v4).

OK -- again, that works with all the previously-crashing test cases.
It's fuzzing now; I'll include it in my series.

Thanks Jan,
 -George


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