Re: [Xen-devel] [PATCH] x86emul: correct loading of %ss

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, 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 )
>      {
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel