Re: [PATCH] Add GDT, IDT and RFLAGS guest state validity checks

[Mohammed Gamal <m.gamal005@gmail.com>]

On Tue, Sep 22, 2009 at 9:13 AM, Avi Kivity <avi@redhat.com> wrote:
> On 09/21/2009 06:33 PM, Mohammed Gamal wrote:
>>
>> With emulate_invalid_guest_state=1 Windows XP exits with an invalid guest
>> state
>> due to rflags not being in a VMX-compliant state. This patch fixes this
>> issue,
>> although Windows XP doesn't boot yet with invalid state emulation on.
>>
>> Also added GDT and IDT checks while we're at it.
>>
>> Signed-off-by: Mohammed Gamal<m.gamal005@gmail.com>
>> ---
>>  arch/x86/kvm/vmx.c |   57
>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 files changed, 56 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 3fe0d42..eaec4a5 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2022,6 +2022,23 @@ static bool ldtr_valid(struct kvm_vcpu *vcpu)
>>        return true;
>>  }
>>
>> +static bool gdtr_idtr_valid(struct kvm_vcpu *vcpu)
>> +{
>> +       struct descriptor_table gdt;
>> +       struct descriptor_table idt;
>> +
>> +       vmx_get_gdt(vcpu,&gdt);
>> +       vmx_get_idt(vcpu,&idt);
>> +
>> +       if (gdt.limit&  0xffff0000)
>> +               return false;
>> +
>> +       if (idt.limit&  0xffff0000)
>> +               return false;
>> +
>> +       return true;
>> +}
>>
>
> gdt and idt limits cannot be > 0xffff, since the intstructions to load them
> always use a 16 bit quantity.
>
>> +
>>  static bool cs_ss_rpl_check(struct kvm_vcpu *vcpu)
>>  {
>>        struct kvm_segment cs, ss;
>> @@ -2033,6 +2050,41 @@ static bool cs_ss_rpl_check(struct kvm_vcpu *vcpu)
>>                 (ss.selector&  SELECTOR_RPL_MASK));
>>  }
>>
>> +static bool rflags_valid(struct kvm_vcpu *vcpu)
>> +{
>> +       unsigned long rflags;
>> +       u32 entry_intr_info;
>> +
>> +       rflags = vmcs_readl(GUEST_RFLAGS);
>> +       entry_intr_info = vmcs_read32(VM_ENTRY_INTR_INFO_FIELD);
>> +#ifdef CONFIG_X86_64
>> +       if (rflags&  0xffffffffffc00000)
>> +               return false;
>> +       if (is_long_mode(vcpu))
>> +               if (rflags&  X86_EFLAGS_VM)
>> +                       return false;
>> +#else
>> +       if (rflags&  0xffc00000)
>> +               return false;
>> +#endif
>> +       if (rflags&  0x8000)
>> +               return false;
>> +       if (rflags&  0x20)
>> +               return false;
>> +       if (rflags&  0x8)
>> +               return false;
>> +       if (!(rflags&  0x2))
>> +               return false;
>> +
>> +       if ((entry_intr_info&  INTR_INFO_INTR_TYPE_MASK) ==
>> INTR_TYPE_EXT_INTR
>> +               &&  (entry_intr_info&  INTR_INFO_VALID_MASK)) {
>> +               if (!(rflags&  X86_EFLAGS_IF))
>> +                       return false;
>> +       }
>> +
>> +       return true;
>> +}
>>
>
> It's really difficult to tell what you are doing here since you're using
> numbers instead of symbolic constants.  But I think some of these are
> generally illegal.  !guest_state_valid() means "the state is valid for x86
> but not valid for vmx entry"; if it's generally invalid all bets are off.
>

The checks are based on those mentioned in the Intel Software
Developer Manual Vol. 2 Section 22.3.1.3 and 22.3.1.4