[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3] x86emul/fuzz: add rudimentary limit checking
>>> On 06.07.17 at 16:02, <george.dunlap@xxxxxxxxxx> wrote: > On 07/06/2017 01:34 PM, Jan Beulich wrote: >>>>> On 06.07.17 at 12:57, <george.dunlap@xxxxxxxxxx> 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@xxxxxxxxxx> >>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> >>>> --- >>>> 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). Jan Attachment:
x86emul-IP-assertion.patch _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |