Re: [Xen-devel] [PATCH] x86emul: correct and extend IDT entry checks

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 06/12/16 11:23, Jan Beulich wrote:
> In order to pre-determine whether a fault will occur upon software
> interrupt injection, it is not sufficient to just check P and DPL. Do
> at least all the checks on the IDT entry itself, and in particular do
> the #NP check last. The checks for the new CS (and perhaps SS) are left
> out for now, though.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxxx>

However, I would like to confirm the GP/NP ordering with an XTF test. 
Given the number of times this has gone wrong in the past, it would be
best not to take chances.

>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -1663,7 +1663,7 @@ static int inject_swint(enum x86_swint_t
>          if ( !in_realmode(ctxt, ops) )
>          {
>              unsigned int idte_size, idte_offset;
> -            struct { uint32_t a, b, c, d; } idte;
> +            struct { uint32_t a, b, c, d; } idte = {};
>              int lm = in_longmode(ctxt, ops);
>  
>              if ( lm < 0 )
> @@ -1708,12 +1708,27 @@ static int inject_swint(enum x86_swint_t
>                  return rc;
>              }
>  
> -            /* Is this entry present? */
> -            if ( !(idte.b & (1u << 15)) )
> +            /* This must be an interrupt, trap, or task gate. */
> +#ifdef __XEN__
> +            switch ( (idte.b >> 8) & 0x1f )
>              {
> -                fault_type = EXC_NP;
> +            case SYS_DESC_irq_gate:
> +            case SYS_DESC_trap_gate:
> +                break;
> +            case SYS_DESC_irq_gate16:
> +            case SYS_DESC_trap_gate16:
> +            case SYS_DESC_task_gate:
> +                if ( !lm )
> +                    break;
> +                /* fall through */
> +            default:
>                  goto raise_exn;
>              }
> +#endif
> +
> +            /* The 64-bit high half's type must be zero. */
> +            if ( idte.d & 0x1f00 )
> +                goto raise_exn;

It's rather odd to have the 64bit check outside of the #if __XEN__. 
Then again, the test harness won't enter this function due to the lack
of x86_seg_idtr, so perhaps it doesn't matter.

~Andrew

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