| Message ID | 57E2697F0200007800110E78@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21/09/16 10:05, Jan Beulich wrote: > - Instead of #NP, #SS needs to be raised for non-present descriptors. > - Loading a null selector is fine in 64-bit mode at CPL != 3, as long > as RPL == CPL. > - Don't lose the low two selector bits on null selector loads (also > applies to %ds, %es, %fs, and %gs). > > Since we need CPL earlier now, also switch to using get_cpl() (instead > of open coding it). > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although... > > --- a/xen/arch/x86/x86_emulate/x86_emulate.c > +++ b/xen/arch/x86/x86_emulate/x86_emulate.c > @@ -1194,18 +1194,25 @@ protmode_load_seg( > struct x86_emulate_ctxt *ctxt, > const struct x86_emulate_ops *ops) > { > - struct segment_register desctab, ss; > + struct segment_register desctab; > struct { uint32_t a, b; } desc; > - uint8_t dpl, rpl, cpl; > + uint8_t dpl, rpl; > + int cpl = get_cpl(ctxt, ops); > uint32_t new_desc_b, a_flag = 0x100; > int rc, fault_type = EXC_GP; > > + if ( cpl < 0 ) > + return X86EMUL_UNHANDLEABLE; > + > /* NULL selector? */ > if ( (sel & 0xfffc) == 0 ) > { > - if ( (seg == x86_seg_cs) || (seg == x86_seg_ss) ) > + if ( (seg == x86_seg_cs) || > + ((seg == x86_seg_ss) && > + (!mode_64bit() || (cpl == 3) || (cpl != sel))) ) > goto raise_exn; > memset(sreg, 0, sizeof(*sreg)); > + sreg->sel = sel; > return X86EMUL_OKAY; > } > > @@ -1213,8 +1220,7 @@ protmode_load_seg( > if ( !is_x86_user_segment(seg) && (sel & 4) ) > goto raise_exn; > > - if ( (rc = ops->read_segment(x86_seg_ss, &ss, ctxt)) || > - (rc = ops->read_segment((sel & 4) ? x86_seg_ldtr : x86_seg_gdtr, > + if ( (rc = ops->read_segment((sel & 4) ? x86_seg_ldtr : x86_seg_gdtr, > &desctab, ctxt)) ) > return rc; > > @@ -1229,7 +1235,7 @@ protmode_load_seg( > /* Segment present in memory? */ > if ( !(desc.b & (1u<<15)) ) > { > - fault_type = EXC_NP; > + fault_type = seg != x86_seg_ss ? EXC_NP : EXC_SS; > goto raise_exn; > } > > @@ -1248,7 +1254,6 @@ protmode_load_seg( > > dpl = (desc.b >> 13) & 3; > rpl = sel & 3; ... it occurs to me that the calculation of rpl can be moved up to its declaration, which allows you to check (cpl == 3) || (cpl != rpl) rather than sel, which is more clearly correct IMO. ~Andrew > - cpl = ss.attr.fields.dpl; > > switch ( seg ) > { > > >
>>> On 26.09.16 at 15:40, <andrew.cooper3@citrix.com> wrote: > On 21/09/16 10:05, Jan Beulich wrote: >> @@ -1248,7 +1254,6 @@ protmode_load_seg( >> >> dpl = (desc.b >> 13) & 3; >> rpl = sel & 3; > > ... it occurs to me that the calculation of rpl can be moved up to its > declaration, which allows you to check (cpl == 3) || (cpl != rpl) rather > than sel, which is more clearly correct IMO. I think I prefer to leave this alone for the moment. Jan
>>> On 26.09.16 at 15:40, <andrew.cooper3@citrix.com> wrote: > On 21/09/16 10:05, Jan Beulich wrote: >> - Instead of #NP, #SS needs to be raised for non-present descriptors. >> - Loading a null selector is fine in 64-bit mode at CPL != 3, as long >> as RPL == CPL. >> - Don't lose the low two selector bits on null selector loads (also >> applies to %ds, %es, %fs, and %gs). >> >> Since we need CPL earlier now, also switch to using get_cpl() (instead >> of open coding it). >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although... > >> >> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >> @@ -1194,18 +1194,25 @@ protmode_load_seg( >> struct x86_emulate_ctxt *ctxt, >> const struct x86_emulate_ops *ops) >> { >> - struct segment_register desctab, ss; >> + struct segment_register desctab; >> struct { uint32_t a, b; } desc; >> - uint8_t dpl, rpl, cpl; >> + uint8_t dpl, rpl; >> + int cpl = get_cpl(ctxt, ops); >> uint32_t new_desc_b, a_flag = 0x100; >> int rc, fault_type = EXC_GP; >> >> + if ( cpl < 0 ) >> + return X86EMUL_UNHANDLEABLE; >> + >> /* NULL selector? */ >> if ( (sel & 0xfffc) == 0 ) >> { >> - if ( (seg == x86_seg_cs) || (seg == x86_seg_ss) ) >> + if ( (seg == x86_seg_cs) || >> + ((seg == x86_seg_ss) && >> + (!mode_64bit() || (cpl == 3) || (cpl != sel))) ) I've just noticed that this depends on @@ -607,7 +609,7 @@ do{ asm volatile ( }) #define truncate_ea(ea) truncate_word((ea), ad_bytes) -#define mode_64bit() (def_ad_bytes == 8) +#define mode_64bit() (ctxt->addr_size == 64) #define fail_if(p) \ do { \ from the large decode rework series. I'll assume you're okay with me folding this in. Jan
On 26/09/16 16:25, Jan Beulich wrote: >>>> On 26.09.16 at 15:40, <andrew.cooper3@citrix.com> wrote: >> On 21/09/16 10:05, Jan Beulich wrote: >>> - Instead of #NP, #SS needs to be raised for non-present descriptors. >>> - Loading a null selector is fine in 64-bit mode at CPL != 3, as long >>> as RPL == CPL. >>> - Don't lose the low two selector bits on null selector loads (also >>> applies to %ds, %es, %fs, and %gs). >>> >>> Since we need CPL earlier now, also switch to using get_cpl() (instead >>> of open coding it). >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although... >> >>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c >>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c >>> @@ -1194,18 +1194,25 @@ protmode_load_seg( >>> struct x86_emulate_ctxt *ctxt, >>> const struct x86_emulate_ops *ops) >>> { >>> - struct segment_register desctab, ss; >>> + struct segment_register desctab; >>> struct { uint32_t a, b; } desc; >>> - uint8_t dpl, rpl, cpl; >>> + uint8_t dpl, rpl; >>> + int cpl = get_cpl(ctxt, ops); >>> uint32_t new_desc_b, a_flag = 0x100; >>> int rc, fault_type = EXC_GP; >>> >>> + if ( cpl < 0 ) >>> + return X86EMUL_UNHANDLEABLE; >>> + >>> /* NULL selector? */ >>> if ( (sel & 0xfffc) == 0 ) >>> { >>> - if ( (seg == x86_seg_cs) || (seg == x86_seg_ss) ) >>> + if ( (seg == x86_seg_cs) || >>> + ((seg == x86_seg_ss) && >>> + (!mode_64bit() || (cpl == 3) || (cpl != sel))) ) > I've just noticed that this depends on > > @@ -607,7 +609,7 @@ do{ asm volatile ( > }) > #define truncate_ea(ea) truncate_word((ea), ad_bytes) > > -#define mode_64bit() (def_ad_bytes == 8) > +#define mode_64bit() (ctxt->addr_size == 64) > > #define fail_if(p) \ > do { \ > > from the large decode rework series. I'll assume you're okay with me > folding this in. Yeah - that looks fine. ~Andrew
--- a/xen/arch/x86/x86_emulate/x86_emulate.c +++ b/xen/arch/x86/x86_emulate/x86_emulate.c @@ -1194,18 +1194,25 @@ protmode_load_seg( struct x86_emulate_ctxt *ctxt, const struct x86_emulate_ops *ops) { - struct segment_register desctab, ss; + struct segment_register desctab; struct { uint32_t a, b; } desc; - uint8_t dpl, rpl, cpl; + uint8_t dpl, rpl; + int cpl = get_cpl(ctxt, ops); uint32_t new_desc_b, a_flag = 0x100; int rc, fault_type = EXC_GP; + if ( cpl < 0 ) + return X86EMUL_UNHANDLEABLE; + /* NULL selector? */ if ( (sel & 0xfffc) == 0 ) { - if ( (seg == x86_seg_cs) || (seg == x86_seg_ss) ) + if ( (seg == x86_seg_cs) || + ((seg == x86_seg_ss) && + (!mode_64bit() || (cpl == 3) || (cpl != sel))) ) goto raise_exn; memset(sreg, 0, sizeof(*sreg)); + sreg->sel = sel; return X86EMUL_OKAY; } @@ -1213,8 +1220,7 @@ protmode_load_seg( if ( !is_x86_user_segment(seg) && (sel & 4) ) goto raise_exn; - if ( (rc = ops->read_segment(x86_seg_ss, &ss, ctxt)) || - (rc = ops->read_segment((sel & 4) ? x86_seg_ldtr : x86_seg_gdtr, + if ( (rc = ops->read_segment((sel & 4) ? x86_seg_ldtr : x86_seg_gdtr, &desctab, ctxt)) ) return rc; @@ -1229,7 +1235,7 @@ protmode_load_seg( /* Segment present in memory? */ if ( !(desc.b & (1u<<15)) ) { - fault_type = EXC_NP; + fault_type = seg != x86_seg_ss ? EXC_NP : EXC_SS; goto raise_exn; } @@ -1248,7 +1254,6 @@ protmode_load_seg( dpl = (desc.b >> 13) & 3; rpl = sel & 3; - cpl = ss.attr.fields.dpl; switch ( seg ) {