* [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC
@ 2016-06-07 10:18 Euan Harris
2016-06-07 10:35 ` Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Euan Harris @ 2016-06-07 10:18 UTC (permalink / raw)
To: xen-devel; +Cc: andrew.cooper3, kevin.tian, Euan Harris, jun.nakajima, jbeulich
Guest reads of MSR_IA32_VMX_VMFUNC should be handled by
the logic in vmx_msr_read_intercept(). Otherwise a guest
can read the raw host value of this MSR, even if nested
vmx is disabled.
Signed-off-by: Euan Harris <euan.harris@citrix.com>
---
xen/arch/x86/hvm/vmx/vmx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 743b5a1..6c0721f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2624,7 +2624,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
__vmread(GUEST_IA32_DEBUGCTL, msr_content);
break;
case IA32_FEATURE_CONTROL_MSR:
- case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS:
+ case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
if ( !nvmx_msr_read_intercept(msr, msr_content) )
goto gp_fault;
break;
--
1.7.10.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC
2016-06-07 10:18 [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC Euan Harris
@ 2016-06-07 10:35 ` Jan Beulich
2016-06-07 10:53 ` Euan Harris
2016-06-08 6:09 ` Tian, Kevin
2016-06-08 11:54 ` Wei Liu
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2016-06-07 10:35 UTC (permalink / raw)
To: Euan Harris; +Cc: andrew.cooper3, kevin.tian, jun.nakajima, xen-devel
>>> On 07.06.16 at 12:18, <euan.harris@citrix.com> wrote:
> Guest reads of MSR_IA32_VMX_VMFUNC should be handled by
> the logic in vmx_msr_read_intercept(). Otherwise a guest
> can read the raw host value of this MSR, even if nested
> vmx is disabled.
>
> Signed-off-by: Euan Harris <euan.harris@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Albeit ...
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2624,7 +2624,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
> __vmread(GUEST_IA32_DEBUGCTL, msr_content);
> break;
> case IA32_FEATURE_CONTROL_MSR:
> - case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS:
> + case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
> if ( !nvmx_msr_read_intercept(msr, msr_content) )
> goto gp_fault;
> break;
... retaining this code structure makes it likely that some future
addition will lead to the same problem again. I think there should
be something like MSR_IA32_VMX_LAST added to msr-index.h,
and get used here instead. Suitably placed it would make pretty
obvious to someone adding a new MSR there that this value also
needs updating.
Or alternatively: Is there an architectural upper bound to the
VMX MSR range? If so, we could widen the set to the full range
in one go.
VMX maintainers - I'd appreciate if you could take care of this in
one way or another.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC
2016-06-07 10:35 ` Jan Beulich
@ 2016-06-07 10:53 ` Euan Harris
2016-06-07 11:39 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Euan Harris @ 2016-06-07 10:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, kevin.tian, jun.nakajima, xen-devel
On Tue, Jun 07, 2016 at 04:35:28AM -0600, Jan Beulich wrote:
> > @@ -2624,7 +2624,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
> > __vmread(GUEST_IA32_DEBUGCTL, msr_content);
> > break;
> > case IA32_FEATURE_CONTROL_MSR:
> > - case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS:
> > + case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
> > if ( !nvmx_msr_read_intercept(msr, msr_content) )
> > goto gp_fault;
> > break;
>
> ... retaining this code structure makes it likely that some future
> addition will lead to the same problem again.
The safest solution would be to whitelist the MSRs which Xen handles and
which the guest should be allowed to see, rather than blacklisting which
is essentially what is happening now. That would involve a substantial
change in the code, but aside from that is there any fundamental reason
why it would be a bad idea?
Thanks,
Euan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC
2016-06-07 10:53 ` Euan Harris
@ 2016-06-07 11:39 ` Andrew Cooper
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2016-06-07 11:39 UTC (permalink / raw)
To: Euan Harris, Jan Beulich; +Cc: xen-devel, kevin.tian, jun.nakajima
On 07/06/16 11:53, Euan Harris wrote:
> On Tue, Jun 07, 2016 at 04:35:28AM -0600, Jan Beulich wrote:
>>> @@ -2624,7 +2624,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>>> __vmread(GUEST_IA32_DEBUGCTL, msr_content);
>>> break;
>>> case IA32_FEATURE_CONTROL_MSR:
>>> - case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS:
>>> + case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
>>> if ( !nvmx_msr_read_intercept(msr, msr_content) )
>>> goto gp_fault;
>>> break;
>> ... retaining this code structure makes it likely that some future
>> addition will lead to the same problem again.
> The safest solution would be to whitelist the MSRs which Xen handles and
> which the guest should be allowed to see, rather than blacklisting which
> is essentially what is happening now. That would involve a substantial
> change in the code, but aside from that is there any fundamental reason
> why it would be a bad idea?
I do have plans which will eventually turn all cpuid information and
msrs visible to guests into a whitelist rather than a blacklist, but
there is indeed a lot of infrastructure work required to make this happen.
It is certainly the longterm plan.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC
2016-06-07 10:18 [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC Euan Harris
2016-06-07 10:35 ` Jan Beulich
@ 2016-06-08 6:09 ` Tian, Kevin
2016-06-08 11:54 ` Wei Liu
2 siblings, 0 replies; 6+ messages in thread
From: Tian, Kevin @ 2016-06-08 6:09 UTC (permalink / raw)
To: Euan Harris, xen-devel; +Cc: andrew.cooper3, Nakajima, Jun, jbeulich
> From: Euan Harris [mailto:euan.harris@citrix.com]
> Sent: Tuesday, June 07, 2016 6:19 PM
>
> Guest reads of MSR_IA32_VMX_VMFUNC should be handled by
> the logic in vmx_msr_read_intercept(). Otherwise a guest
> can read the raw host value of this MSR, even if nested
> vmx is disabled.
>
> Signed-off-by: Euan Harris <euan.harris@citrix.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC
2016-06-07 10:18 [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC Euan Harris
2016-06-07 10:35 ` Jan Beulich
2016-06-08 6:09 ` Tian, Kevin
@ 2016-06-08 11:54 ` Wei Liu
2 siblings, 0 replies; 6+ messages in thread
From: Wei Liu @ 2016-06-08 11:54 UTC (permalink / raw)
To: Euan Harris
Cc: kevin.tian, Wei Liu, jun.nakajima, andrew.cooper3, jbeulich, xen-devel
On Tue, Jun 07, 2016 at 10:18:38AM +0000, Euan Harris wrote:
> Guest reads of MSR_IA32_VMX_VMFUNC should be handled by
> the logic in vmx_msr_read_intercept(). Otherwise a guest
> can read the raw host value of this MSR, even if nested
> vmx is disabled.
>
> Signed-off-by: Euan Harris <euan.harris@citrix.com>
For 4.7:
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
> ---
> xen/arch/x86/hvm/vmx/vmx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 743b5a1..6c0721f 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -2624,7 +2624,7 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
> __vmread(GUEST_IA32_DEBUGCTL, msr_content);
> break;
> case IA32_FEATURE_CONTROL_MSR:
> - case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_TRUE_ENTRY_CTLS:
> + case MSR_IA32_VMX_BASIC...MSR_IA32_VMX_VMFUNC:
> if ( !nvmx_msr_read_intercept(msr, msr_content) )
> goto gp_fault;
> break;
> --
> 1.7.10.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-08 11:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-07 10:18 [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC Euan Harris
2016-06-07 10:35 ` Jan Beulich
2016-06-07 10:53 ` Euan Harris
2016-06-07 11:39 ` Andrew Cooper
2016-06-08 6:09 ` Tian, Kevin
2016-06-08 11:54 ` Wei Liu
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).