Re: [Xen-devel] [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC

[Wei Liu]

On Tue, Jun 07, 2016 at 10:18:38AM +, 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 

For 4.7:

Release-acked-by: Wei Liu 

> ---
>  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

Re: [Xen-devel] [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC

[Tian, Kevin]

> From: Euan Harris [mailto:euan.har...@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 

Acked-by: Kevin Tian 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC

[Andrew Cooper]

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

Re: [Xen-devel] [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC

[Euan Harris]

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

Re: [Xen-devel] [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC

[Jan Beulich]

>>> On 07.06.16 at 12:18,  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 

Reviewed-by: Jan Beulich 

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

[Xen-devel] [PATCH] nested vmx: Intercept guest rdmsr for MSR_IA32_VMX_VMFUNC

[Euan Harris]

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 
---
 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