[PATCH] SVM: forbid guest to execute monitor/mwait

[PATCH] SVM: forbid guest to execute monitor/mwait

[Joerg Roedel]

[-- Attachment #1: Type: text/plain, Size: 427 bytes --]

From: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>

This patch forbids the guest to execute monitor/mwait instructions on
SVM. This is necessary because the guest can execute these instructions
if they are available even if the kvm cpuid doesn't report its
existence.

Signed-off-by: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
-- 
Joerg Roedel
Operating System Research Center
AMD Saxony LLC & Co. KG

[-- Attachment #2: kvm-svm-monitor-disable.patch --]
[-- Type: text/plain, Size: 1430 bytes --]

diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
index d3cc115..68a2520 100644
--- a/drivers/kvm/svm.c
+++ b/drivers/kvm/svm.c
@@ -511,7 +511,9 @@ static void init_vmcb(struct vmcb *vmcb)
 				(1ULL << INTERCEPT_VMSAVE) |
 				(1ULL << INTERCEPT_STGI) |
 				(1ULL << INTERCEPT_CLGI) |
-				(1ULL << INTERCEPT_SKINIT);
+				(1ULL << INTERCEPT_SKINIT) |
+				(1ULL << INTERCEPT_MONITOR) |
+				(1ULL << INTERCEPT_MWAIT);
 
 	control->iopm_base_pa = iopm_base;
 	control->msrpm_base_pa = msrpm_base;
@@ -1292,6 +1294,8 @@ static int (*svm_exit_handlers[])(struct kvm_vcpu *vcpu,
 	[SVM_EXIT_STGI]				= invalid_op_interception,
 	[SVM_EXIT_CLGI]				= invalid_op_interception,
 	[SVM_EXIT_SKINIT]			= invalid_op_interception,
+	[SVM_EXIT_MONITOR]			= invalid_op_interception,
+	[SVM_EXIT_MWAIT]			= invalid_op_interception,
 };
 
 
diff --git a/drivers/kvm/svm.h b/drivers/kvm/svm.h
index df731c3..5e93814 100644
--- a/drivers/kvm/svm.h
+++ b/drivers/kvm/svm.h
@@ -44,6 +44,9 @@ enum {
 	INTERCEPT_RDTSCP,
 	INTERCEPT_ICEBP,
 	INTERCEPT_WBINVD,
+	INTERCEPT_MONITOR,
+	INTERCEPT_MWAIT,
+	INTERCEPT_MWAIT_COND,
 };
 
 
@@ -298,6 +301,9 @@ struct __attribute__ ((__packed__)) vmcb {
 #define SVM_EXIT_RDTSCP		0x087
 #define SVM_EXIT_ICEBP		0x088
 #define SVM_EXIT_WBINVD		0x089
+#define SVM_EXIT_MONITOR	0x08a
+#define SVM_EXIT_MWAIT		0x08b
+#define SVM_EXIT_MWAIT_COND	0x08c
 #define SVM_EXIT_NPF  		0x400
 
 #define SVM_EXIT_ERR		-1

[-- Attachment #3: Type: text/plain, Size: 345 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Avi Kivity]

Joerg Roedel wrote:
> From: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
>
> This patch forbids the guest to execute monitor/mwait instructions on
> SVM. This is necessary because the guest can execute these instructions
> if they are available even if the kvm cpuid doesn't report its
> existence.
>
>   

You're intercepting the instructions unconditionally.  What about the 
case where cpuid does report monitor/mwait support?  The guest can 
legitimately use them then.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Joerg Roedel]

On Thu, Mar 22, 2007 at 09:38:12AM +0200, Avi Kivity wrote:
> Joerg Roedel wrote:
> >From: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
> >
> >This patch forbids the guest to execute monitor/mwait instructions on
> >SVM. This is necessary because the guest can execute these instructions
> >if they are available even if the kvm cpuid doesn't report its
> >existence.
> >
> >  
> 
> You're intercepting the instructions unconditionally.  What about the case where cpuid does 
> report monitor/mwait support?  The guest can legitimately use them then.

I prepared a patch for QEmu to handle this but finally not
submitted it because QEmu does not set the Monitor bit anyway. But you
are right. It is possible for userspace to set the Montior bit and the
guest won't be able to use it.
I think an architecture dependent cpuid mask feature in the kernel would
solve this problem. I'll prepare a patch for that.

Joerg

-- 
Joerg Roedel
Operating System Research Center
AMD Saxony LLC & Co. KG



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Avi Kivity]

Joerg Roedel wrote:
> On Thu, Mar 22, 2007 at 09:38:12AM +0200, Avi Kivity wrote:
>   
>> Joerg Roedel wrote:
>>     
>>> From: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
>>>
>>> This patch forbids the guest to execute monitor/mwait instructions on
>>> SVM. This is necessary because the guest can execute these instructions
>>> if they are available even if the kvm cpuid doesn't report its
>>> existence.
>>>
>>>  
>>>       
>> You're intercepting the instructions unconditionally.  What about the case where cpuid does 
>> report monitor/mwait support?  The guest can legitimately use them then.
>>     
>
> I prepared a patch for QEmu to handle this but finally not
> submitted it because QEmu does not set the Monitor bit anyway. But you
> are right. It is possible for userspace to set the Montior bit and the
> guest won't be able to use it.
> I think an architecture dependent cpuid mask feature in the kernel would
> solve this problem. I'll prepare a patch for that.
>
>   

I think I'm misunderstanding something.

Is there actually an AMD cpu that supports monitor/mwait?  If not 
(understandably, having no hyperthreading), then wouldn't executing 
monitor or mwait generate #UD anyway?  I don't think there's need for 
the cpuid mask as long as setting the monitor bit cannot endanger the 
kernel.



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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Joerg Roedel]

On Thu, Mar 22, 2007 at 12:06:32PM +0200, Avi Kivity wrote:
> Joerg Roedel wrote:
> >On Thu, Mar 22, 2007 at 09:38:12AM +0200, Avi Kivity wrote:
> >  
> >>Joerg Roedel wrote:
> >>    
> >>>From: Joerg Roedel <joerg.roedel-5C7GfCeVMHo@public.gmane.org>
> >>>
> >>>This patch forbids the guest to execute monitor/mwait instructions on
> >>>SVM. This is necessary because the guest can execute these instructions
> >>>if they are available even if the kvm cpuid doesn't report its
> >>>existence.
> >>>
> >>>       
> >>You're intercepting the instructions unconditionally.  What about the case where cpuid does 
> >>report monitor/mwait support?  The guest can legitimately use them then.
> >>    
> >
> >I prepared a patch for QEmu to handle this but finally not
> >submitted it because QEmu does not set the Monitor bit anyway. But you
> >are right. It is possible for userspace to set the Montior bit and the
> >guest won't be able to use it.
> >I think an architecture dependent cpuid mask feature in the kernel would
> >solve this problem. I'll prepare a patch for that.
> >
> >  
> 
> I think I'm misunderstanding something.
> 
> Is there actually an AMD cpu that supports monitor/mwait?  If not (understandably, having no 

Yes, the family 10h processors have support for that.

> hyperthreading), then wouldn't executing monitor or mwait generate #UD anyway?  I don't think 
> there's need for the cpuid mask as long as setting the monitor bit cannot endanger the 
> kernel.

There is no danger for the host kernel but for the guest. If the
userspace sets the monitor bit the guest will receive an #UD when trying
to use it. And we don't want the guest to use it because it is not
virtualized yet.

Joerg

-- 
Joerg Roedel
Operating System Research Center
AMD Saxony LLC & Co. KG



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Avi Kivity]

Joerg Roedel wrote:
> There is no danger for the host kernel but for the guest. If the
> userspace sets the monitor bit the guest will receive an #UD when trying
> to use it. And we don't want the guest to use it because it is not
> virtualized yet.
>   

So, isn't a simpler fix not to set the monitor bit in cpuid?

The patch is correct, but I don't see why it's needed as any guest will 
check the cpuid bit before using monitor.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Joerg Roedel]

On Thu, Mar 22, 2007 at 12:42:28PM +0200, Avi Kivity wrote:
> Joerg Roedel wrote:
> >There is no danger for the host kernel but for the guest. If the
> >userspace sets the monitor bit the guest will receive an #UD when trying
> >to use it. And we don't want the guest to use it because it is not
> >virtualized yet.
> >  
> 
> So, isn't a simpler fix not to set the monitor bit in cpuid?
> 
> The patch is correct, but I don't see why it's needed as any guest will check the cpuid bit 
> before using monitor.

Right. But it is possible for userspace to enable monitor bit for the
guest. Without virtualization of these 2 instructions the guest would
idle in the guest state after calling mwait and prevent other processes
and guests from running in that time. I don't think this is acceptable.
And if we disable this bit in cpuid we should also prevent the
guest from executing it to emulate the behavior of a real cpu in host
mode.
So I agree we should mask the cpuid monitor bit. The savest way to do so
is in the kvm-amd module imho. And, also imho, we should the guest
really forbid to execute it.

Joerg

-- 
Joerg Roedel
Operating System Research Center
AMD Saxony LLC & Co. KG



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Avi Kivity]

Joerg Roedel wrote:
> On Thu, Mar 22, 2007 at 12:42:28PM +0200, Avi Kivity wrote:
>   
>> Joerg Roedel wrote:
>>     
>>> There is no danger for the host kernel but for the guest. If the
>>> userspace sets the monitor bit the guest will receive an #UD when trying
>>> to use it. And we don't want the guest to use it because it is not
>>> virtualized yet.
>>>  
>>>       
>> So, isn't a simpler fix not to set the monitor bit in cpuid?
>>
>> The patch is correct, but I don't see why it's needed as any guest will check the cpuid bit 
>> before using monitor.
>>     
>
> Right. But it is possible for userspace to enable monitor bit for the
> guest. Without virtualization of these 2 instructions the guest would
> idle in the guest state after calling mwait and prevent other processes
> and guests from running in that time. 

Won't an interrupt during mwait cause a vmexit?

> I don't think this is acceptable.
> And if we disable this bit in cpuid we should also prevent the
> guest from executing it to emulate the behavior of a real cpu in host
> mode.
> So I agree we should mask the cpuid monitor bit. The savest way to do so
> is in the kvm-amd module imho. And, also imho, we should the guest
> really forbid to execute it.
>   

I want to understand it exactly first.  I don't see any barrier to 
applying the patch, I just want a full understanding.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Joerg Roedel]

On Thu, Mar 22, 2007 at 01:40:26PM +0200, Avi Kivity wrote:
> Joerg Roedel wrote:
> >Right. But it is possible for userspace to enable monitor bit for the
> >guest. Without virtualization of these 2 instructions the guest would
> >idle in the guest state after calling mwait and prevent other processes
> >and guests from running in that time. 
> 
> Won't an interrupt during mwait cause a vmexit?

Yes, an interrupt would cause an vmexit. But depending on the HZ value
of the host it may be a lot of time until the next interrupt. And over
time this will sum up and slow down other tasks.

Joerg

-- 
Joerg Roedel
Operating System Research Center
AMD Saxony LLC & Co. KG



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Avi Kivity]

Joerg Roedel wrote:
> On Thu, Mar 22, 2007 at 01:40:26PM +0200, Avi Kivity wrote:
>   
>> Joerg Roedel wrote:
>>     
>>> Right. But it is possible for userspace to enable monitor bit for the
>>> guest. Without virtualization of these 2 instructions the guest would
>>> idle in the guest state after calling mwait and prevent other processes
>>> and guests from running in that time. 
>>>       
>> Won't an interrupt during mwait cause a vmexit?
>>     
>
> Yes, an interrupt would cause an vmexit. But depending on the HZ value
> of the host it may be a lot of time until the next interrupt. And over
> time this will sum up and slow down other tasks.
>   

Well, a guest can cause exactly the same effect with

    cli
spin:
    jmp spin

without executing any instructions that are not reported by cpuid.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Joerg Roedel]

On Thu, Mar 22, 2007 at 02:01:18PM +0200, Avi Kivity wrote:
> Joerg Roedel wrote:
> >On Thu, Mar 22, 2007 at 01:40:26PM +0200, Avi Kivity wrote:
> >  
> >>Joerg Roedel wrote:
> >>    
> >>>Right. But it is possible for userspace to enable monitor bit for the
> >>>guest. Without virtualization of these 2 instructions the guest would
> >>>idle in the guest state after calling mwait and prevent other processes
> >>>and guests from running in that time.       
> >>Won't an interrupt during mwait cause a vmexit?
> >>    
> >
> >Yes, an interrupt would cause an vmexit. But depending on the HZ value
> >of the host it may be a lot of time until the next interrupt. And over
> >time this will sum up and slow down other tasks.
> >  
> 
> Well, a guest can cause exactly the same effect with
> 
>    cli
> spin:
>    jmp spin
> 
> without executing any instructions that are not reported by cpuid.

Intentionally, yes. In this case the guest knows what it is doing and we
have no way to prevent such things anyway. If it executes mwait this
delay loop is not intended by the guest.

To summarize my point:

We agree that we should hide the monitor cpuid bit to the guest from the
kernel level. Allowing the guest to use mwait if its available on the
processor without proper virtualization would cause the processor to
idle in the guest mode.
I would additionally forbid the guest to execute these instructions.
Likely the guest won't use them anyway when the cpuid bit is not set,
but if this bit is not set, the execution of these instructions should
cause an #UD as in any real processor, imho.

Joerg

-- 
Joerg Roedel
Operating System Research Center
AMD Saxony LLC & Co. KG



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Avi Kivity]

Joerg Roedel wrote:
> On Thu, Mar 22, 2007 at 02:01:18PM +0200, Avi Kivity wrote:
>   
>> Joerg Roedel wrote:
>>     
>>> On Thu, Mar 22, 2007 at 01:40:26PM +0200, Avi Kivity wrote:
>>>  
>>>       
>>>> Joerg Roedel wrote:
>>>>    
>>>>         
>>>>> Right. But it is possible for userspace to enable monitor bit for the
>>>>> guest. Without virtualization of these 2 instructions the guest would
>>>>> idle in the guest state after calling mwait and prevent other processes
>>>>> and guests from running in that time.       
>>>>>           
>>>> Won't an interrupt during mwait cause a vmexit?
>>>>    
>>>>         
>>> Yes, an interrupt would cause an vmexit. But depending on the HZ value
>>> of the host it may be a lot of time until the next interrupt. And over
>>> time this will sum up and slow down other tasks.
>>>  
>>>       
>> Well, a guest can cause exactly the same effect with
>>
>>    cli
>> spin:
>>    jmp spin
>>
>> without executing any instructions that are not reported by cpuid.
>>     
>
> Intentionally, yes. In this case the guest knows what it is doing and we
> have no way to prevent such things anyway. If it executes mwait this
> delay loop is not intended by the guest.
>   

Well, if the cpuid bit is off and the guest is executing the 
instruction, then it probably isn't doing it intentionally...

> To summarize my point:
>
> We agree that we should hide the monitor cpuid bit to the guest from the
> kernel level. Allowing the guest to use mwait if its available on the
> processor without proper virtualization would cause the processor to
> idle in the guest mode.
>   

I think it can be safely done from userspace, as long as host integrity 
is not compromised.

> I would additionally forbid the guest to execute these instructions.
> Likely the guest won't use them anyway when the cpuid bit is not set,
> but if this bit is not set, the execution of these instructions should
> cause an #UD as in any real processor, imho.
>   

I agree that it's better, but it's just marginally better.  There are 
zillions of instructions that are advertised by cpuid that won't be 
trapped by svm (or vt) if the virtual cpuid bit is off.

Anyway, as the patch does improve things a bit, I'll apply it.

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


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: [PATCH] SVM: forbid guest to execute monitor/mwait

[Joerg Roedel]

On Thu, Mar 22, 2007 at 02:33:50PM +0200, Avi Kivity wrote:
> Joerg Roedel wrote:
> >We agree that we should hide the monitor cpuid bit to the guest from the
> >kernel level. Allowing the guest to use mwait if its available on the
> >processor without proper virtualization would cause the processor to
> >idle in the guest mode.
> >  
> 
> I think it can be safely done from userspace, as long as host integrity is not compromised.

Ok. This way is also most efficient because it does not require any
further patching :) QEmu currently does not set this bit.

-- 
Joerg Roedel
Operating System Research Center
AMD Saxony LLC & Co. KG



-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV