| Message ID | 1465294718-31626-1-git-send-email-euan.harris@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
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
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
> 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>
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
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;
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(-)