| Message ID | 595CD4410200007800168ACF@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/07/17 10:57, 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 son after complete_insn, which collides with the soon > 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. > > Reported-by: George Dunlap <george.dunlap@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c > +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c > @@ -139,7 +139,10 @@ 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 > + assert(is_x86_system_segment(seg) && !(offset >> 48)); Why 48? For GDTR/IDTR, the limit is explicitly 16 bits. For LDTR, the limit is 32 bits, but only 16 bits worth of offset can be architecturally reached. For TR, the limit is also 32 bits, but only 17 bits can be be reached (given some specific IO_BITMAP_OFFSET choices). Everything else looks fine, though. ~Andrew
>>> On 05.07.17 at 12:31, <andrew.cooper3@citrix.com> wrote: > On 05/07/17 10:57, 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 son after complete_insn, which collides with the > > soon > >> 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. >> >> Reported-by: George Dunlap <george.dunlap@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c >> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c >> @@ -139,7 +139,10 @@ 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 >> + assert(is_x86_system_segment(seg) && !(offset >> 48)); > > Why 48? > > For GDTR/IDTR, the limit is explicitly 16 bits. Oops - I've simply counted from the wrong end, subtracting the 16 from 64. Jan
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c @@ -139,7 +139,10 @@ 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 + assert(is_x86_system_segment(seg) && !(offset >> 48)); return data_read(ctxt, seg, "read", p_data, bytes); } @@ -162,6 +165,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 +242,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 +258,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 +272,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 +290,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 +304,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 +321,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 >> 48)); return maybe_fail(ctxt, "cmpxchg", true); } @@ -319,6 +336,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); }
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 son 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. Reported-by: George Dunlap <george.dunlap@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com>