Re: [PATCH] VMX: Invalid guest state detection enhancements and bug fixes
On Tue, May 11, 2010 at 01:53:51PM +0300, Mohammed Gamal wrote:
> >> 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.
Userspace can be extended to print instruction at RIP on emulation
failure. Furthermore userspace can even disassemble it for you. IIRC
kvm_report_emulation_failure() was once removed from here and I've
actually sent patch to remove kvm_report_emulation_failure() at all.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VMX: Invalid guest state detection enhancements and bug fixes
On Tue, May 11, 2010 at 12:24 PM, Avi Kivity 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
>
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VMX: Invalid guest state detection enhancements and bug fixes
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?
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.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] VMX: Invalid guest state detection enhancements and bug fixes
- Correct unusable flag check on SS, DS, ES, FS, GS, and LDTR
- Add rflags checks
- Report failed instruction on emulation failure
Signed-off-by: Mohammed Gamal
---
arch/x86/kvm/vmx.c | 31 +++
1 files changed, 27 insertions(+), 4 deletions(-)
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;
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)
@@ -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)
@@ -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
+ 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;
+}
+
/*
* Check if guest state is valid. Returns true if valid, false if
* not.
@@ -2251,8 +2272,9 @@ static bool guest_state_valid(struct kvm_vcpu *vcpu)
}
/* TODO:
* - Add checks on RIP
-* - Add checks on RFLAGS
*/
+ if (!rflags_valid(vcpu))
+ return false;
return true;
}
@@ -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;
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] VMX: Invalid guest state detection enhancements and bug fixes
- Correct unusable flag check on SS, DS, ES, FS, GS, and LDTR
- Add IDTR and GDTR checks
- Add rflags checks
Signed-off-by: Mohammed Gamal
---
arch/x86/kvm/vmx.c | 64 ---
1 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 777e00d..f736008 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;
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)
@@ -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)
@@ -2196,6 +2196,23 @@ static bool ldtr_valid(struct kvm_vcpu *vcpu)
return true;
}
+static bool gdtr_idtr_valid(struct kvm_vcpu *vcpu)
+{
+ struct desc_ptr gdt;
+ struct desc_ptr idt;
+
+ vmx_get_gdt(vcpu, &gdt);
+ vmx_get_idt(vcpu, &idt);
+
+ if (gdt.size & 0x)
+ return false;
+
+ if (idt.size & 0x)
+ return false;
+
+ return true;
+}
+
static bool cs_ss_rpl_check(struct kvm_vcpu *vcpu)
{
struct kvm_segment cs, ss;
@@ -2207,6 +2224,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 & 0xffc0)
+ return false;
+ if (is_long_mode(vcpu))
+ if (rflags & X86_EFLAGS_VM)
+ return false;
+#else
+ if (rflags & 0xffc0)
+ 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;
+}
+
/*
* Check if guest state is valid. Returns true if valid, false if
* not.
@@ -2251,8 +2303,11 @@ static bool guest_state_valid(struct kvm_vcpu *vcpu)
}
/* TODO:
* - Add checks on RIP
-* - Add checks on RFLAGS
*/
+ if (!rflags_valid(vcpu))
+ return false;
+ if (!gdtr_idtr_valid(vcpu))
+ return false;
return true;
}
@@ -3559,6 +3614,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;
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
