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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.12.16 at 14:36, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

So would you prefer me to wait with committing this until you have
that test in place?

>> --- 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.

Well, the conditional is there just to prevent a build failure (due to
the missing SYS_DESC_* definitions). Once we've found a reasonable
way to make those arch definitions re-usable, that conditional could
go away.

Jan


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