| Message ID | 1481714459-6593-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 14.12.16 at 12:20, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -2765,6 +2765,7 @@ x86_emulate( > if ( mode_64bit() && (op_bytes == 4) ) > op_bytes = 8; > seg = (b >> 3) & 7; > + ASSERT(is_x86_user_segment(seg)); Kind of pointless, don't you think? It's guaranteed by the set of case statements ahead of here. > @@ -3393,25 +3394,32 @@ x86_emulate( > _regs.eip = dst.val; > break; > > - case 0xc4: /* les */ { > + case 0xc4: /* les */ > + case 0xc5: /* lds */ > + { > unsigned long sel; Since you're touching this anyway, and since you're eliminating the use of dst.val here, may I ask that you eliminate this local variable at once, using dst.val instead? > - dst.val = x86_seg_es; > - les: /* dst.val identifies the segment */ > - generate_exception_if(mode_64bit() && !ext, EXC_UD); > + > + generate_exception_if(mode_64bit(), EXC_UD); > + seg = (b & 1) * 3; /* es = 0, ds = 3 */ > + goto les; > + > + case X86EMUL_OPC(0x0f, 0xb2): /* lss */ > + case X86EMUL_OPC(0x0f, 0xb4): /* lfs */ > + case X86EMUL_OPC(0x0f, 0xb5): /* lgs */ > + seg = b & 7; > + > + les: My general line of thinking about where to place case labels was - next to each other for opcodes sharing all of their code, or allowing plain fall-through, - in numeric order when plain fall-through is not possible. Hence I'd prefer if the three could stay at the place where LSS was. Jan
On 14/12/16 13:34, Jan Beulich wrote: >>>> On 14.12.16 at 12:20, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -2765,6 +2765,7 @@ x86_emulate( >> if ( mode_64bit() && (op_bytes == 4) ) >> op_bytes = 8; >> seg = (b >> 3) & 7; >> + ASSERT(is_x86_user_segment(seg)); > Kind of pointless, don't you think? It's guaranteed by the set of > case statements ahead of here. Well, I didn't think so at the time. All other uses either have seg used from a constant, or has previously had a user check in a generate_exception_if() clause. > >> @@ -3393,25 +3394,32 @@ x86_emulate( >> _regs.eip = dst.val; >> break; >> >> - case 0xc4: /* les */ { >> + case 0xc4: /* les */ >> + case 0xc5: /* lds */ >> + { >> unsigned long sel; > Since you're touching this anyway, and since you're eliminating the > use of dst.val here, may I ask that you eliminate this local variable > at once, using dst.val instead? Good point. > >> - dst.val = x86_seg_es; >> - les: /* dst.val identifies the segment */ >> - generate_exception_if(mode_64bit() && !ext, EXC_UD); >> + >> + generate_exception_if(mode_64bit(), EXC_UD); >> + seg = (b & 1) * 3; /* es = 0, ds = 3 */ >> + goto les; >> + >> + case X86EMUL_OPC(0x0f, 0xb2): /* lss */ >> + case X86EMUL_OPC(0x0f, 0xb4): /* lfs */ >> + case X86EMUL_OPC(0x0f, 0xb5): /* lgs */ >> + seg = b & 7; >> + >> + les: > My general line of thinking about where to place case labels was > - next to each other for opcodes sharing all of their code, or allowing > plain fall-through, > - in numeric order when plain fall-through is not possible. > Hence I'd prefer if the three could stay at the place where LSS was. Ok. ~Andrew
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c index 1b5becf..2fb99e9 100644 --- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -2765,6 +2765,7 @@ x86_emulate( if ( mode_64bit() && (op_bytes == 4) ) op_bytes = 8; seg = (b >> 3) & 7; + ASSERT(is_x86_user_segment(seg)); if ( (rc = read_ulong(x86_seg_ss, sp_post_inc(op_bytes), &dst.val, op_bytes, ctxt, ops)) != X86EMUL_OKAY || (rc = load_seg(seg, dst.val, 0, NULL, ctxt, ops)) != X86EMUL_OKAY ) @@ -3393,25 +3394,32 @@ x86_emulate( _regs.eip = dst.val; break; - case 0xc4: /* les */ { + case 0xc4: /* les */ + case 0xc5: /* lds */ + { unsigned long sel; - dst.val = x86_seg_es; - les: /* dst.val identifies the segment */ - generate_exception_if(mode_64bit() && !ext, EXC_UD); + + generate_exception_if(mode_64bit(), EXC_UD); + seg = (b & 1) * 3; /* es = 0, ds = 3 */ + goto les; + + case X86EMUL_OPC(0x0f, 0xb2): /* lss */ + case X86EMUL_OPC(0x0f, 0xb4): /* lfs */ + case X86EMUL_OPC(0x0f, 0xb5): /* lgs */ + seg = b & 7; + + les: generate_exception_if(src.type != OP_MEM, EXC_UD); if ( (rc = read_ulong(src.mem.seg, src.mem.off + src.bytes, - &sel, 2, ctxt, ops)) != 0 ) + &sel, 2, ctxt, ops)) != X86EMUL_OKAY ) goto done; - if ( (rc = load_seg(dst.val, sel, 0, NULL, ctxt, ops)) != 0 ) + ASSERT(is_x86_user_segment(seg)); + if ( (rc = load_seg(seg, sel, 0, NULL, ctxt, ops)) != X86EMUL_OKAY ) goto done; dst.val = src.val; break; } - case 0xc5: /* lds */ - dst.val = x86_seg_ds; - goto les; - case 0xc8: /* enter imm16,imm8 */ { uint8_t depth = imm2 & 31; int i; @@ -5228,22 +5236,10 @@ x86_emulate( } break; - case X86EMUL_OPC(0x0f, 0xb2): /* lss */ - dst.val = x86_seg_ss; - goto les; - case X86EMUL_OPC(0x0f, 0xb3): btr: /* btr */ emulate_2op_SrcV_nobyte("btr", src, dst, _regs.eflags); break; - case X86EMUL_OPC(0x0f, 0xb4): /* lfs */ - dst.val = x86_seg_fs; - goto les; - - case X86EMUL_OPC(0x0f, 0xb5): /* lgs */ - dst.val = x86_seg_gs; - goto les; - case X86EMUL_OPC(0x0f, 0xb6): /* movzx rm8,r{16,32,64} */ /* Recompute DstReg as we may have decoded AH/BH/CH/DH. */ dst.reg = decode_register(modrm_reg, &_regs, 0);
%ss, %fs and %gs can be calculated by directly masking the opcode. %es and %ds cant, but the calculation isn't hard. Use seg rather than dst.val for storing the calculated segment, which is appropriately typed. The mode_64() check can be repositioned and simplified to drop the ext check. Replace opencoding of X86EMUL_OKAY. Finally, introduce assertions each time we calculate a user segment to load (rather than using constants) which don't have other validity checks. This includes the POP %sreg case. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> --- xen/arch/x86/x86_emulate/x86_emulate.c | 40 +++++++++++++++------------------- 1 file changed, 18 insertions(+), 22 deletions(-)