Re: [PATCH] VMX: Invalid guest state detection enhancements and bug fixes

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

On Tue, May 11, 2010 at 12:24 PM, Avi Kivity <avi@redhat.com> wrote:
> On 05/10/2010 06:51 PM, Mohammed Gamal wrote:
>>
>> - Correct unusable flag check on SS, DS, ES, FS, GS, and LDTR
>> - Add rflags checks
>> - Report failed instruction on emulation failure
>>
>>
>
> Please post as separate patches.
>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 777e00d..968384b 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2122,7 +2122,7 @@ static bool stack_segment_valid(struct kvm_vcpu
>> *vcpu)
>>        ss_rpl = ss.selector&  SELECTOR_RPL_MASK;
>>
>>        if (ss.unusable)
>> -               return true;
>> +               return false;
>>
>
> Where does it say that ss must be usable?

Oh, I must have misread it. The unusable bit check is for virtual 8086
mode. However, with this check returning true when the segment is
unusable we still return prematurely as we don't do the other checks.

>
>>        if (ss.type != 3&&  ss.type != 7)
>>                return false;
>>        if (!ss.s)
>> @@ -2144,7 +2144,7 @@ static bool data_segment_valid(struct kvm_vcpu
>> *vcpu, int seg)
>>        rpl = var.selector&  SELECTOR_RPL_MASK;
>>
>>        if (var.unusable)
>> -               return true;
>> +               return false;
>>        if (!var.s)
>>                return false;
>>        if (!var.present)
>>
>
> Ditto.
>
>> @@ -2185,7 +2185,7 @@ static bool ldtr_valid(struct kvm_vcpu *vcpu)
>>        vmx_get_segment(vcpu,&ldtr, VCPU_SREG_LDTR);
>>
>>        if (ldtr.unusable)
>> -               return true;
>> +               return false;
>>        if (ldtr.selector&  SELECTOR_TI_MASK)   /* TI = 1 */
>>                return false;
>>        if (ldtr.type != 2)
>>
>
> Ditto.
>
>> @@ -2207,6 +2207,27 @@ 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 (is_long_mode(vcpu))
>> +               if (rflags&  X86_EFLAGS_VM)
>> +                       return false;
>> +#endif
>>
>
> This is architecturally illegal, not just for vmx entries.  The check should
> be made when emulating setting rflags and in KVM_SET_REGS.
>
> These checks should assume the state is architecturally legal, and only
> check if they are legal for vmx entries.
>

>> +       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;
>> +       }
>> +
>>
>
> Ditto.
>
>> @@ -3559,6 +3581,7 @@ static int handle_invalid_guest_state(struct
>> kvm_vcpu *vcpu)
>>                }
>>
>>                if (err != EMULATE_DONE) {
>> +                       kvm_report_emulation_failure(vcpu, "invalid guest
>> state handler");
>>                        vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
>>                        vcpu->run->internal.suberror =
>> KVM_INTERNAL_ERROR_EMULATION;
>>                        vcpu->run->internal.ndata = 0;
>>
>
> Uneeded, userspace can report it.

Userspace does report the address at which a VM fails, it doesn't for
instance show the contents of RIP which is necessary for knowing which
instruction failed, this is at least needed for testing purposes. IIRC
I've seen some trace_kvm_failed_insn() functions somewhere. But I
don't know how to use tracing utilities so I'd be delighted if someone
could point me to some relevant documentation or something.

>
> --
> error compiling committee.c: too many arguments to function
>
>