[PATCH] KVM: handle exit due to INVD in VMX

[PATCH] KVM: handle exit due to INVD in VMX

[Gleb Natapov]

Call into emulator when INVD instruction is executed by a guest.

Signed-off-by: Gleb Natapov <gleb@redhat.com>
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 9f0cbd9..42d9590 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -239,6 +239,7 @@ enum vmcs_field {
 #define EXIT_REASON_TASK_SWITCH         9
 #define EXIT_REASON_CPUID               10
 #define EXIT_REASON_HLT                 12
+#define EXIT_REASON_INVD                13
 #define EXIT_REASON_INVLPG              14
 #define EXIT_REASON_RDPMC               15
 #define EXIT_REASON_RDTSC               16
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 993e332..200533e 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3350,6 +3350,11 @@ static int handle_vmx_insn(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static int handle_invd(struct kvm_vcpu *vcpu)
+{
+	return emulate_instruction(vcpu, 0, 0, 0) == EMULATE_DONE;
+}
+
 static int handle_invlpg(struct kvm_vcpu *vcpu)
 {
 	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
@@ -3650,6 +3655,7 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
 	[EXIT_REASON_MSR_WRITE]               = handle_wrmsr,
 	[EXIT_REASON_PENDING_INTERRUPT]       = handle_interrupt_window,
 	[EXIT_REASON_HLT]                     = handle_halt,
+	[EXIT_REASON_INVD]		      = handle_invd,
 	[EXIT_REASON_INVLPG]		      = handle_invlpg,
 	[EXIT_REASON_VMCALL]                  = handle_vmcall,
 	[EXIT_REASON_VMCLEAR]	              = handle_vmx_insn,
--
			Gleb.

Re: [PATCH] KVM: handle exit due to INVD in VMX

[Alexander Graf]

On 31.10.2010, at 07:36, Gleb Natapov wrote:

> Call into emulator when INVD instruction is executed by a guest.

Why? This is a poor patch description.

Alex

Re: [PATCH] KVM: handle exit due to INVD in VMX

[Gleb Natapov]

On Sun, Oct 31, 2010 at 11:00:08AM -0700, Alexander Graf wrote:
> 
> On 31.10.2010, at 07:36, Gleb Natapov wrote:
> 
> > Call into emulator when INVD instruction is executed by a guest.
> 
> Why? This is a poor patch description.
Why what? Why we need to handle INVD exit instead of stopping with
unhandled exit error?

--
			Gleb.

Re: [PATCH] KVM: handle exit due to INVD in VMX

[Alexander Graf]

On 31.10.2010, at 11:22, Gleb Natapov wrote:

> On Sun, Oct 31, 2010 at 11:00:08AM -0700, Alexander Graf wrote:
>> 
>> On 31.10.2010, at 07:36, Gleb Natapov wrote:
>> 
>>> Call into emulator when INVD instruction is executed by a guest.
>> 
>> Why? This is a poor patch description.
> Why what? Why we need to handle INVD exit instead of stopping with
> unhandled exit error?

Ah, so we get the exit already, but don't handle it? That's an important piece of information that belongs in the patch description. Another thing I as a reader would also like to know is where this got triggered, so which guests would break without the patch.

I'm also wondering why nobody has seen it before. Is this a regression? Is this exit a side-effect of another feature bit of VMX, so only newer CPUs are affected?


Alex

Re: [PATCH] KVM: handle exit due to INVD in VMX

[Gleb Natapov]

On Sun, Oct 31, 2010 at 11:26:09AM -0700, Alexander Graf wrote:
> 
> On 31.10.2010, at 11:22, Gleb Natapov wrote:
> 
> > On Sun, Oct 31, 2010 at 11:00:08AM -0700, Alexander Graf wrote:
> >> 
> >> On 31.10.2010, at 07:36, Gleb Natapov wrote:
> >> 
> >>> Call into emulator when INVD instruction is executed by a guest.
> >> 
> >> Why? This is a poor patch description.
> > Why what? Why we need to handle INVD exit instead of stopping with
> > unhandled exit error?
> 
> Ah, so we get the exit already, but don't handle it? That's an important piece of information that belongs in the patch description. Another thing I as a reader would also like to know is where this got triggered, so which guests would break without the patch.
> 
I'll add it to the patch description. The guest that triggered it was
open firmware, but I do not think this info belongs to patch description
too.

> I'm also wondering why nobody has seen it before. Is this a regression? Is this exit a side-effect of another feature bit of VMX, so only newer CPUs are affected?
> 
I guess nobody seen it because not many guests use the instruction.
Actually this instruction is useful only for firmware use. This is not a
regression.

--
			Gleb.

Re: [PATCH] KVM: handle exit due to INVD in VMX

[Alexander Graf]

On 31.10.2010, at 11:56, Gleb Natapov wrote:

> On Sun, Oct 31, 2010 at 11:26:09AM -0700, Alexander Graf wrote:
>> 
>> On 31.10.2010, at 11:22, Gleb Natapov wrote:
>> 
>>> On Sun, Oct 31, 2010 at 11:00:08AM -0700, Alexander Graf wrote:
>>>> 
>>>> On 31.10.2010, at 07:36, Gleb Natapov wrote:
>>>> 
>>>>> Call into emulator when INVD instruction is executed by a guest.
>>>> 
>>>> Why? This is a poor patch description.
>>> Why what? Why we need to handle INVD exit instead of stopping with
>>> unhandled exit error?
>> 
>> Ah, so we get the exit already, but don't handle it? That's an important piece of information that belongs in the patch description. Another thing I as a reader would also like to know is where this got triggered, so which guests would break without the patch.
>> 
> I'll add it to the patch description. The guest that triggered it was
> open firmware, but I do not think this info belongs to patch description
> too.

Quite the contrary, I would be very interested in that information in the patch description. The patch description is what people afterwards use to cherry-pick patches. So this is crucial.

> 
>> I'm also wondering why nobody has seen it before. Is this a regression? Is this exit a side-effect of another feature bit of VMX, so only newer CPUs are affected?
>> 
> I guess nobody seen it because not many guests use the instruction.
> Actually this instruction is useful only for firmware use. This is not a
> regression.

This, too, should go in the patch description :). At least the part that usually only firmware uses it. The part where it has been around since the beginning might be interesting as well from a security point of view. After all, the guest can kill its full kvm context without going through qemu interfaces.


Thanks!

Alex

Re: [PATCH] KVM: handle exit due to INVD in VMX

[Gleb Natapov]

On Sun, Oct 31, 2010 at 12:01:29PM -0700, Alexander Graf wrote:
> 
> On 31.10.2010, at 11:56, Gleb Natapov wrote:
> 
> > On Sun, Oct 31, 2010 at 11:26:09AM -0700, Alexander Graf wrote:
> >> 
> >> On 31.10.2010, at 11:22, Gleb Natapov wrote:
> >> 
> >>> On Sun, Oct 31, 2010 at 11:00:08AM -0700, Alexander Graf wrote:
> >>>> 
> >>>> On 31.10.2010, at 07:36, Gleb Natapov wrote:
> >>>> 
> >>>>> Call into emulator when INVD instruction is executed by a guest.
> >>>> 
> >>>> Why? This is a poor patch description.
> >>> Why what? Why we need to handle INVD exit instead of stopping with
> >>> unhandled exit error?
> >> 
> >> Ah, so we get the exit already, but don't handle it? That's an important piece of information that belongs in the patch description. Another thing I as a reader would also like to know is where this got triggered, so which guests would break without the patch.
> >> 
> > I'll add it to the patch description. The guest that triggered it was
> > open firmware, but I do not think this info belongs to patch description
> > too.
> 
> Quite the contrary, I would be very interested in that information in the patch description. The patch description is what people afterwards use to cherry-pick patches. So this is crucial.
> 
> > 
> >> I'm also wondering why nobody has seen it before. Is this a regression? Is this exit a side-effect of another feature bit of VMX, so only newer CPUs are affected?
> >> 
> > I guess nobody seen it because not many guests use the instruction.
> > Actually this instruction is useful only for firmware use. This is not a
> > regression.
> 
> This, too, should go in the patch description :). At least the part that usually only firmware uses it. The part where it has been around since the beginning might be interesting as well from a security point of view. After all, the guest can kill its full kvm context without going through qemu interfaces.
> 
> 
A guest has thousands ways to kill qemu from ring 0. Actually, after
fixing the bug, open firmware kills qemu in other place. This time it
looks like it maps two pci bars into the same physical address for a
short time (one of then framebuffer) and this kills qemu-kvm dirty
logging (qemu works).

--
			Gleb.