* [PATCH] VMX: Invalid guest state detection enhancements and bug fixes
@ 2010-05-09 20:45 Mohammed Gamal
0 siblings, 0 replies; 5+ messages in thread
From: Mohammed Gamal @ 2010-05-09 20:45 UTC (permalink / raw)
To: avi; +Cc: kvm, Mohammed Gamal
- 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 <m.gamal005@gmail.com>
---
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 & 0xffff0000)
+ return false;
+
+ if (idt.size & 0xffff0000)
+ 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 & 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;
+}
+
/*
* 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] VMX: Invalid guest state detection enhancements and bug fixes
@ 2010-05-10 15:51 Mohammed Gamal
2010-05-11 9:24 ` Avi Kivity
0 siblings, 1 reply; 5+ messages in thread
From: Mohammed Gamal @ 2010-05-10 15:51 UTC (permalink / raw)
To: avi; +Cc: mtosatti, kvm, Mohammed Gamal
- 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 <m.gamal005@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] VMX: Invalid guest state detection enhancements and bug fixes
2010-05-10 15:51 [PATCH] VMX: Invalid guest state detection enhancements and bug fixes Mohammed Gamal
@ 2010-05-11 9:24 ` Avi Kivity
2010-05-11 10:53 ` Mohammed Gamal
0 siblings, 1 reply; 5+ messages in thread
From: Avi Kivity @ 2010-05-11 9:24 UTC (permalink / raw)
To: Mohammed Gamal; +Cc: mtosatti, kvm
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] VMX: Invalid guest state detection enhancements and bug fixes
2010-05-11 9:24 ` Avi Kivity
@ 2010-05-11 10:53 ` Mohammed Gamal
2010-05-11 11:06 ` Gleb Natapov
0 siblings, 1 reply; 5+ messages in thread
From: Mohammed Gamal @ 2010-05-11 10:53 UTC (permalink / raw)
To: Avi Kivity; +Cc: mtosatti, kvm
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] VMX: Invalid guest state detection enhancements and bug fixes
2010-05-11 10:53 ` Mohammed Gamal
@ 2010-05-11 11:06 ` Gleb Natapov
0 siblings, 0 replies; 5+ messages in thread
From: Gleb Natapov @ 2010-05-11 11:06 UTC (permalink / raw)
To: Mohammed Gamal; +Cc: Avi Kivity, mtosatti, kvm
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-05-11 11:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-10 15:51 [PATCH] VMX: Invalid guest state detection enhancements and bug fixes Mohammed Gamal
2010-05-11 9:24 ` Avi Kivity
2010-05-11 10:53 ` Mohammed Gamal
2010-05-11 11:06 ` Gleb Natapov
-- strict thread matches above, loose matches on Subject: below --
2010-05-09 20:45 Mohammed Gamal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).