* [PATCH v3] x86emul/fuzz: add rudimentary limit checking
@ 2017-07-06 9:20 Jan Beulich
2017-07-06 10:57 ` George Dunlap
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-07-06 9:20 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper
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.
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -139,7 +139,17 @@ 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) && !(offset >> 16));
return data_read(ctxt, seg, "read", p_data, bytes);
}
@@ -162,6 +172,13 @@ static int fuzz_insn_fetch(
{
assert(seg == x86_seg_cs);
+ /* Minimal segment limit checking, until full one is being put in place. */
+ if ( ctxt->addr_size < 64 && (offset >> 32) )
+ {
+ x86_emul_hw_exception(13, 0, ctxt);
+ return X86EMUL_EXCEPTION;
+ }
+
/*
* Zero-length instruction fetches are made at the destination of jumps,
* to perform segmentation checks. No data needs returning.
@@ -232,6 +249,7 @@ static int fuzz_rep_ins(
struct x86_emulate_ctxt *ctxt)
{
assert(dst_seg == x86_seg_es);
+ assert(ctxt->addr_size == 64 || !(dst_offset >> 32));
return _fuzz_rep_read(ctxt, "rep_ins", reps);
}
@@ -247,6 +265,7 @@ static int fuzz_rep_movs(
{
assert(is_x86_user_segment(src_seg));
assert(dst_seg == x86_seg_es);
+ assert(ctxt->addr_size == 64 || !((src_offset | dst_offset) >> 32));
return _fuzz_rep_read(ctxt, "rep_movs", reps);
}
@@ -260,6 +279,7 @@ static int fuzz_rep_outs(
struct x86_emulate_ctxt *ctxt)
{
assert(is_x86_user_segment(src_seg));
+ assert(ctxt->addr_size == 64 || !(src_offset >> 32));
return _fuzz_rep_write(ctxt, "rep_outs", reps);
}
@@ -277,6 +297,7 @@ static int fuzz_rep_stos(
* for CLZERO.
*/
assert(is_x86_user_segment(seg));
+ assert(ctxt->addr_size == 64 || !(offset >> 32));
return _fuzz_rep_write(ctxt, "rep_stos", reps);
}
@@ -290,6 +311,7 @@ static int fuzz_write(
{
/* Writes not expected for any system segments. */
assert(is_x86_user_segment(seg));
+ assert(ctxt->addr_size == 64 || !(offset >> 32));
return maybe_fail(ctxt, "write", true);
}
@@ -306,8 +328,10 @@ static int fuzz_cmpxchg(
* Cmpxchg expected for user segments, and setting accessed/busy bits in
* GDT/LDT enties, but not expected for any IDT or TR accesses.
*/
- assert(is_x86_user_segment(seg) ||
- seg == x86_seg_gdtr || seg == x86_seg_ldtr);
+ if ( is_x86_user_segment(seg) )
+ assert(ctxt->addr_size == 64 || !(offset >> 32));
+ else
+ assert((seg == x86_seg_gdtr || seg == x86_seg_ldtr) && !(offset >> 16));
return maybe_fail(ctxt, "cmpxchg", true);
}
@@ -319,6 +343,7 @@ static int fuzz_invlpg(
{
/* invlpg(), unlike all other hooks, may be called with x86_seg_none. */
assert(is_x86_user_segment(seg) || seg == x86_seg_none);
+ assert(ctxt->addr_size == 64 || !(offset >> 32));
return maybe_fail(ctxt, "invlpg", false);
}
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1249,10 +1249,10 @@ static void __put_rep_prefix(
/* Clip maximum repetitions so that the index register at most just wraps. */
#define truncate_ea_and_reps(ea, reps, bytes_per_rep) ({ \
- unsigned long todo__, ea__ = truncate_word(ea, ad_bytes); \
+ unsigned long todo__, ea__ = truncate_ea(ea); \
if ( !(_regs.eflags & X86_EFLAGS_DF) ) \
- todo__ = truncate_word(-(ea), ad_bytes) / (bytes_per_rep); \
- else if ( truncate_word((ea) + (bytes_per_rep) - 1, ad_bytes) < ea__ )\
+ todo__ = truncate_ea(-ea__) / (bytes_per_rep); \
+ else if ( truncate_ea(ea__ + (bytes_per_rep) - 1) < ea__ ) \
todo__ = 1; \
else \
todo__ = ea__ / (bytes_per_rep) + 1; \
@@ -3128,6 +3128,7 @@ x86_emulate(
op_bytes + (((-src.val - 1) >> 3) & ~(op_bytes - 1L));
else
ea.mem.off += (src.val >> 3) & ~(op_bytes - 1L);
+ ea.mem.off = truncate_ea(ea.mem.off);
}
/* Bit index always truncated to within range. */
@@ -3346,7 +3347,7 @@ x86_emulate(
unsigned long src_val2;
int lb, ub, idx;
generate_exception_if(src.type != OP_MEM, EXC_UD);
- if ( (rc = read_ulong(src.mem.seg, src.mem.off + op_bytes,
+ if ( (rc = read_ulong(src.mem.seg, truncate_ea(src.mem.off + op_bytes),
&src_val2, op_bytes, ctxt, ops)) )
goto done;
ub = (op_bytes == 2) ? (int16_t)src_val2 : (int32_t)src_val2;
@@ -3897,7 +3898,7 @@ x86_emulate(
seg = (b & 1) * 3; /* es = 0, ds = 3 */
les:
generate_exception_if(src.type != OP_MEM, EXC_UD);
- if ( (rc = read_ulong(src.mem.seg, src.mem.off + src.bytes,
+ if ( (rc = read_ulong(src.mem.seg, truncate_ea(src.mem.off + src.bytes),
&dst.val, 2, ctxt, ops)) != X86EMUL_OKAY )
goto done;
ASSERT(is_x86_user_segment(seg));
@@ -4931,7 +4932,8 @@ x86_emulate(
case 5: /* jmp (far, absolute indirect) */
generate_exception_if(src.type != OP_MEM, EXC_UD);
- if ( (rc = read_ulong(src.mem.seg, src.mem.off + op_bytes,
+ if ( (rc = read_ulong(src.mem.seg,
+ truncate_ea(src.mem.off + op_bytes),
&imm2, 2, ctxt, ops)) )
goto done;
imm1 = src.val;
@@ -5115,8 +5117,8 @@ x86_emulate(
}
if ( (rc = ops->write(ea.mem.seg, ea.mem.off, &sreg.limit,
2, ctxt)) != X86EMUL_OKAY ||
- (rc = ops->write(ea.mem.seg, ea.mem.off + 2, &sreg.base,
- op_bytes, ctxt)) != X86EMUL_OKAY )
+ (rc = ops->write(ea.mem.seg, truncate_ea(ea.mem.off + 2),
+ &sreg.base, op_bytes, ctxt)) != X86EMUL_OKAY )
goto done;
break;
case 2: /* lgdt */
@@ -5125,9 +5127,9 @@ x86_emulate(
generate_exception_if(ea.type != OP_MEM, EXC_UD);
fail_if(ops->write_segment == NULL);
memset(&sreg, 0, sizeof(sreg));
- if ( (rc = read_ulong(ea.mem.seg, ea.mem.off+0,
+ if ( (rc = read_ulong(ea.mem.seg, ea.mem.off,
&limit, 2, ctxt, ops)) ||
- (rc = read_ulong(ea.mem.seg, ea.mem.off+2,
+ (rc = read_ulong(ea.mem.seg, truncate_ea(ea.mem.off + 2),
&base, mode_64bit() ? 8 : 4, ctxt, ops)) )
goto done;
generate_exception_if(!is_canonical_address(base), EXC_GP, 0);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] x86emul/fuzz: add rudimentary limit checking
2017-07-06 9:20 [PATCH v3] x86emul/fuzz: add rudimentary limit checking Jan Beulich
@ 2017-07-06 10:57 ` George Dunlap
2017-07-06 12:34 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: George Dunlap @ 2017-07-06 10:57 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: George Dunlap, Andrew Cooper
[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]
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()).
-George
[-- Attachment #2: protmode_load_seg_crash.txt --]
[-- Type: text/plain, Size: 5085 bytes --]
Setting MSR i3 (c0000080) to 1ad40d032ceb49
Setting CR 4 to 22030d031aeb36
Setting CR 0 to ffff001ae95b0000
Canonicalized 0 to 0
Setting EFER_LMA
Disabling hook cmpxchg
Disabling hook rep_outs
Disabling hook rep_stos
Disabling hook read_io
Disabling hook write_io
Disabling hook write_cr
Disabling hook invlpg
Setting EFER_LMA
Setting EFER_LMA
-- State --
addr / sp size: 16 / 16
cr0: ffff001ae95b0001
cr3: 0
cr4: 22030d031aeb36
rip: 0
Setting EFER_LMA
EFER: 1ad40d032cef49
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: c9
maybe_fail read: X86EMUL_OKAY
read: 03 ff
Emulation result: 0
Setting EFER_LMA
Setting EFER_LMA
-- State --
addr / sp size: 16 / 16
cr0: ffff001ae95b0001
cr3: 0
cr4: 22030d031aeb36
rip: 1
Setting EFER_LMA
EFER: 1ad40d032cef49
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: f3
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 0f
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 05
Setting EFER_LMA
maybe_fail write_segment: X86EMUL_OKAY
maybe_fail write_segment: X86EMUL_OKAY
Emulation result: 0
Setting EFER_LMA
Setting EFER_LMA
-- State --
addr / sp size: 64 / 64
cr0: ffff001ae95b0001
cr3: 0
cr4: 22030d031aeb36
rip: 0
Setting EFER_LMA
EFER: 1ad40d032cef49
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 4c
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 4c
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 4c
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 0f
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: ac
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 30
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 03
maybe_fail read: X86EMUL_OKAY
read: 4c 4c 2b 0d b6 80 18 c9
maybe_fail write: X86EMUL_OKAY
Emulation result: 0
Setting EFER_LMA
Setting EFER_LMA
-- State --
addr / sp size: 64 / 64
cr0: ffff001ae95b0001
cr3: 0
cr4: 22030d031aeb36
rip: 7
Setting EFER_LMA
EFER: 1ad40d032cef49
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: ff
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: f3
maybe_fail write: X86EMUL_OKAY
Emulation result: 0
Setting EFER_LMA
Setting EFER_LMA
-- State --
addr / sp size: 64 / 64
cr0: ffff001ae95b0001
cr3: 0
cr4: 22030d031aeb36
rip: 9
Setting EFER_LMA
EFER: 1ad40d032cef49
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 01
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 00
maybe_fail read: X86EMUL_OKAY
read: 64 4c 6a 4c
Emulation result: 0
Setting EFER_LMA
Setting EFER_LMA
-- State --
addr / sp size: 64 / 64
cr0: ffff001ae95b0001
cr3: 0
cr4: 22030d031aeb36
rip: b
Setting EFER_LMA
EFER: 1ad40d032cef49
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 4c
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 0f
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: 03
maybe_fail insn_fetch: X86EMUL_OKAY
insn_fetch: d4
maybe_fail read: X86EMUL_OKAY
read: 00 00 00 4c 37 4c 4c 77
afl-harness: fuzz-emul.c:177: int fuzz_read(enum x86_segment, unsigned long, void *, unsigned int, struct x86_emulate_ctxt *): Assertion `is_x86_system_segment(seg) && !(offset >> 16)' failed.
Program received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
#1 0x00007ffff7a6e3fa in __GI_abort () at abort.c:89
#2 0x00007ffff7a65e37 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x443082 "is_x86_system_segment(seg) && !(offset >> 16)", file=file@entry=0x442fde "fuzz-emul.c", line=line@entry=177, function=function@entry=0x442fea "int fuzz_read(enum x86_segment, unsigned long, void *, unsigned int, struct x86_emulate_ctxt *)") at assert.c:92
#3 0x00007ffff7a65ee2 in __GI___assert_fail (assertion=0x443082 "is_x86_system_segment(seg) && !(offset >> 16)", file=0x442fde "fuzz-emul.c", line=177, function=0x442fea "int fuzz_read(enum x86_segment, unsigned long, void *, unsigned int, struct x86_emulate_ctxt *)") at assert.c:101
#4 0x0000000000403b7e in fuzz_read (seg=<optimized out>, offset=<optimized out>, p_data=0x8, bytes=4294955200, ctxt=0x0) at fuzz-emul.c:177
#5 0x0000000000441afa in protmode_load_seg (seg=<optimized out>, sel=<optimized out>, is_ret=<error reading variable: access outside bounds of object referenced via synthetic pointer>, sreg=<optimized out>, ctxt=<optimized out>, ops=<optimized out>) at ./x86_emulate/x86_emulate.c:1824
#6 0x000000000041888f in x86_emulate (ctxt=<optimized out>, ops=<optimized out>) at ./x86_emulate/x86_emulate.c:5238
#7 0x000000000044240b in x86_emulate_wrapper (ctxt=0x7fffffffe320, ops=0x7fffffffd0c0) at ./x86_emulate/x86_emulate.c:7921
#8 0x00000000004028b4 in runtest (state=<optimized out>, ctxt=<optimized out>) at fuzz-emul.c:911
#9 0x0000000000402ae4 in LLVMFuzzerTestOneInput (data_p=0x6571c0 <input> "\250\254\020\067\003\367\025\016\b", size=112) at fuzz-emul.c:949
#10 0x0000000000401418 in main (argc=<optimized out>, argv=<optimized out>) at afl-harness.c:108
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] x86emul/fuzz: add rudimentary limit checking
2017-07-06 10:57 ` George Dunlap
@ 2017-07-06 12:34 ` Jan Beulich
2017-07-06 14:02 ` George Dunlap
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-07-06 12:34 UTC (permalink / raw)
To: George Dunlap; +Cc: George Dunlap, Andrew Cooper, xen-devel
>>> 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).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] x86emul/fuzz: add rudimentary limit checking
2017-07-06 12:34 ` Jan Beulich
@ 2017-07-06 14:02 ` George Dunlap
2017-07-06 15:21 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: George Dunlap @ 2017-07-06 14:02 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, Andrew Cooper, xen-devel
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.
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] x86emul/fuzz: add rudimentary limit checking
2017-07-06 14:02 ` George Dunlap
@ 2017-07-06 15:21 ` Jan Beulich
2017-07-06 15:28 ` George Dunlap
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2017-07-06 15:21 UTC (permalink / raw)
To: George Dunlap; +Cc: George Dunlap, Andrew Cooper, xen-devel
[-- Attachment #1: Type: text/plain, Size: 2654 bytes --]
>>> 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).
Jan
[-- Attachment #2: x86emul-IP-assertion.patch --]
[-- Type: text/plain, Size: 9237 bytes --]
x86emul/fuzz: add rudimentary limit checking
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>
---
v4: Relax system segment read upper bounds for long mode.
v3: Add more truncate_ea().
v2: Correct system segment related assert()-s.
--- 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)));
return data_read(ctxt, seg, "read", p_data, bytes);
}
@@ -162,6 +173,13 @@ static int fuzz_insn_fetch(
{
assert(seg == x86_seg_cs);
+ /* Minimal segment limit checking, until full one is being put in place. */
+ if ( ctxt->addr_size < 64 && (offset >> 32) )
+ {
+ x86_emul_hw_exception(13, 0, ctxt);
+ return X86EMUL_EXCEPTION;
+ }
+
/*
* Zero-length instruction fetches are made at the destination of jumps,
* to perform segmentation checks. No data needs returning.
@@ -232,6 +250,7 @@ static int fuzz_rep_ins(
struct x86_emulate_ctxt *ctxt)
{
assert(dst_seg == x86_seg_es);
+ assert(ctxt->addr_size == 64 || !(dst_offset >> 32));
return _fuzz_rep_read(ctxt, "rep_ins", reps);
}
@@ -247,6 +266,7 @@ static int fuzz_rep_movs(
{
assert(is_x86_user_segment(src_seg));
assert(dst_seg == x86_seg_es);
+ assert(ctxt->addr_size == 64 || !((src_offset | dst_offset) >> 32));
return _fuzz_rep_read(ctxt, "rep_movs", reps);
}
@@ -260,6 +280,7 @@ static int fuzz_rep_outs(
struct x86_emulate_ctxt *ctxt)
{
assert(is_x86_user_segment(src_seg));
+ assert(ctxt->addr_size == 64 || !(src_offset >> 32));
return _fuzz_rep_write(ctxt, "rep_outs", reps);
}
@@ -277,6 +298,7 @@ static int fuzz_rep_stos(
* for CLZERO.
*/
assert(is_x86_user_segment(seg));
+ assert(ctxt->addr_size == 64 || !(offset >> 32));
return _fuzz_rep_write(ctxt, "rep_stos", reps);
}
@@ -290,6 +312,7 @@ static int fuzz_write(
{
/* Writes not expected for any system segments. */
assert(is_x86_user_segment(seg));
+ assert(ctxt->addr_size == 64 || !(offset >> 32));
return maybe_fail(ctxt, "write", true);
}
@@ -306,8 +329,10 @@ static int fuzz_cmpxchg(
* Cmpxchg expected for user segments, and setting accessed/busy bits in
* GDT/LDT enties, but not expected for any IDT or TR accesses.
*/
- assert(is_x86_user_segment(seg) ||
- seg == x86_seg_gdtr || seg == x86_seg_ldtr);
+ if ( is_x86_user_segment(seg) )
+ assert(ctxt->addr_size == 64 || !(offset >> 32));
+ else
+ assert((seg == x86_seg_gdtr || seg == x86_seg_ldtr) && !(offset >> 16));
return maybe_fail(ctxt, "cmpxchg", true);
}
@@ -319,6 +344,7 @@ static int fuzz_invlpg(
{
/* invlpg(), unlike all other hooks, may be called with x86_seg_none. */
assert(is_x86_user_segment(seg) || seg == x86_seg_none);
+ assert(ctxt->addr_size == 64 || !(offset >> 32));
return maybe_fail(ctxt, "invlpg", false);
}
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1249,10 +1249,10 @@ static void __put_rep_prefix(
/* Clip maximum repetitions so that the index register at most just wraps. */
#define truncate_ea_and_reps(ea, reps, bytes_per_rep) ({ \
- unsigned long todo__, ea__ = truncate_word(ea, ad_bytes); \
+ unsigned long todo__, ea__ = truncate_ea(ea); \
if ( !(_regs.eflags & X86_EFLAGS_DF) ) \
- todo__ = truncate_word(-(ea), ad_bytes) / (bytes_per_rep); \
- else if ( truncate_word((ea) + (bytes_per_rep) - 1, ad_bytes) < ea__ )\
+ todo__ = truncate_ea(-ea__) / (bytes_per_rep); \
+ else if ( truncate_ea(ea__ + (bytes_per_rep) - 1) < ea__ ) \
todo__ = 1; \
else \
todo__ = ea__ / (bytes_per_rep) + 1; \
@@ -3128,6 +3128,7 @@ x86_emulate(
op_bytes + (((-src.val - 1) >> 3) & ~(op_bytes - 1L));
else
ea.mem.off += (src.val >> 3) & ~(op_bytes - 1L);
+ ea.mem.off = truncate_ea(ea.mem.off);
}
/* Bit index always truncated to within range. */
@@ -3346,7 +3347,7 @@ x86_emulate(
unsigned long src_val2;
int lb, ub, idx;
generate_exception_if(src.type != OP_MEM, EXC_UD);
- if ( (rc = read_ulong(src.mem.seg, src.mem.off + op_bytes,
+ if ( (rc = read_ulong(src.mem.seg, truncate_ea(src.mem.off + op_bytes),
&src_val2, op_bytes, ctxt, ops)) )
goto done;
ub = (op_bytes == 2) ? (int16_t)src_val2 : (int32_t)src_val2;
@@ -3897,7 +3898,7 @@ x86_emulate(
seg = (b & 1) * 3; /* es = 0, ds = 3 */
les:
generate_exception_if(src.type != OP_MEM, EXC_UD);
- if ( (rc = read_ulong(src.mem.seg, src.mem.off + src.bytes,
+ if ( (rc = read_ulong(src.mem.seg, truncate_ea(src.mem.off + src.bytes),
&dst.val, 2, ctxt, ops)) != X86EMUL_OKAY )
goto done;
ASSERT(is_x86_user_segment(seg));
@@ -4931,7 +4932,8 @@ x86_emulate(
case 5: /* jmp (far, absolute indirect) */
generate_exception_if(src.type != OP_MEM, EXC_UD);
- if ( (rc = read_ulong(src.mem.seg, src.mem.off + op_bytes,
+ if ( (rc = read_ulong(src.mem.seg,
+ truncate_ea(src.mem.off + op_bytes),
&imm2, 2, ctxt, ops)) )
goto done;
imm1 = src.val;
@@ -5115,8 +5117,8 @@ x86_emulate(
}
if ( (rc = ops->write(ea.mem.seg, ea.mem.off, &sreg.limit,
2, ctxt)) != X86EMUL_OKAY ||
- (rc = ops->write(ea.mem.seg, ea.mem.off + 2, &sreg.base,
- op_bytes, ctxt)) != X86EMUL_OKAY )
+ (rc = ops->write(ea.mem.seg, truncate_ea(ea.mem.off + 2),
+ &sreg.base, op_bytes, ctxt)) != X86EMUL_OKAY )
goto done;
break;
case 2: /* lgdt */
@@ -5125,9 +5127,9 @@ x86_emulate(
generate_exception_if(ea.type != OP_MEM, EXC_UD);
fail_if(ops->write_segment == NULL);
memset(&sreg, 0, sizeof(sreg));
- if ( (rc = read_ulong(ea.mem.seg, ea.mem.off+0,
+ if ( (rc = read_ulong(ea.mem.seg, ea.mem.off,
&limit, 2, ctxt, ops)) ||
- (rc = read_ulong(ea.mem.seg, ea.mem.off+2,
+ (rc = read_ulong(ea.mem.seg, truncate_ea(ea.mem.off + 2),
&base, mode_64bit() ? 8 : 4, ctxt, ops)) )
goto done;
generate_exception_if(!is_canonical_address(base), EXC_GP, 0);
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] x86emul/fuzz: add rudimentary limit checking
2017-07-06 15:21 ` Jan Beulich
@ 2017-07-06 15:28 ` George Dunlap
0 siblings, 0 replies; 6+ messages in thread
From: George Dunlap @ 2017-07-06 15:28 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, Andrew Cooper, xen-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-07-06 15:29 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-06 9:20 [PATCH v3] x86emul/fuzz: add rudimentary limit checking Jan Beulich
2017-07-06 10:57 ` George Dunlap
2017-07-06 12:34 ` Jan Beulich
2017-07-06 14:02 ` George Dunlap
2017-07-06 15:21 ` Jan Beulich
2017-07-06 15:28 ` George Dunlap
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).