Re: [PATCH] x86: defer not-present segment checks

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 06/10/16 13:24, Jan Beulich wrote:
> Following on from commits 5602e74c60 ("x86emul: correct loading of
> %ss") and bdb860d01c ("x86/HVM: correct segment register loading during
> task switch") the point of the non-.present checks needs to be refined:
> #NP (and its #SS companion), other than suggested by the various
> instruction pages in Intel's SDM, gets checked for only after all type
> and permission checks. The only checks getting done even later are the
> 64-bit specific ones for system descriptors (which we don't support
> yet).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Testing confirms this to be correct.  However, there is one issue...

> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1311,7 +1311,7 @@ protmode_load_seg(
>      struct { uint32_t a, b; } desc;
>      uint8_t dpl, rpl;
>      int cpl = get_cpl(ctxt, ops);
> -    uint32_t new_desc_b, a_flag = 0x100;
> +    uint32_t a_flag = 0x100;
>      int rc, fault_type = EXC_GP;
>  
>      if ( cpl < 0 )
> @@ -1352,13 +1352,6 @@ protmode_load_seg(
>                           &desc, sizeof(desc), ctxt)) )
>          return rc;
>  
> -    /* Segment present in memory? */
> -    if ( !(desc.b & (1u<<15)) )
> -    {
> -        fault_type = seg != x86_seg_ss ? EXC_NP : EXC_SS;
> -        goto raise_exn;
> -    }
> -
>      if ( !is_x86_user_segment(seg) )
>      {
>          /* System segments must have S flag == 0. */
> @@ -1410,7 +1403,8 @@ protmode_load_seg(
>          /* LDT system segment? */
>          if ( (desc.b & (15u<<8)) != (2u<<8) )
>              goto raise_exn;
> -        goto skip_accessed_flag;
> +        a_flag = 0;
> +        break;
>      case x86_seg_tr:
>          /* Available TSS system segment? */
>          if ( (desc.b & (15u<<8)) != (9u<<8) )
> @@ -1428,18 +1422,26 @@ protmode_load_seg(

Inbetween these two hunks lives the 64bit %cs check for L and D.  Until
P is checked and found to be set, these bits are available for software use.

The check should be moved down until after the presence check.

Otherwise, everything else looks fine.

~Andrew

>          break;
>      }
>  
> +    /* Segment present in memory? */
> +    if ( !(desc.b & (1u<<15)) )
> +    {
> +        fault_type = seg != x86_seg_ss ? EXC_NP : EXC_SS;
> +        goto raise_exn;
> +    }
> +
>      /* Ensure Accessed flag is set. */
> -    new_desc_b = desc.b | a_flag;
> -    if ( !(desc.b & a_flag) &&
> -         ((rc = ops->cmpxchg(
> -             x86_seg_none, desctab.base + (sel & 0xfff8) + 4,
> -             &desc.b, &new_desc_b, 4, ctxt)) != 0) )
> -        return rc;
> +    if ( a_flag && !(desc.b & a_flag) )
> +    {
> +        uint32_t new_desc_b = desc.b | a_flag;
>  
> -    /* Force the Accessed flag in our local copy. */
> -    desc.b |= a_flag;
> +        if ( (rc = ops->cmpxchg(x86_seg_none, desctab.base + (sel & 0xfff8) + 4,
> +                                &desc.b, &new_desc_b, 4, ctxt)) != 0 )
> +            return rc;
> +
> +        /* Force the Accessed flag in our local copy. */
> +        desc.b = new_desc_b;
> +    }
>  
> - skip_accessed_flag:
>      sreg->base = (((desc.b <<  0) & 0xff000000u) |
>                    ((desc.b << 16) & 0x00ff0000u) |
>                    ((desc.a >> 16) & 0x0000ffffu));
>
>


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