[PATCH 1/3] kvm: vmx: Do not disable intercepts for BNDCFGS

[PATCH 1/3] kvm: vmx: Do not disable intercepts for BNDCFGS

[Jim Mattson]

The MSR permission bitmaps are shared by all VMs. However, some VMs
may not be configured to support MPX, even when the host does. If the
host supports VMX and the guest does not, we should intercept accesses
to the BNDCFGS MSR, so that we can synthesize a #GP
fault. Furthermore, if the host does not support MPX and the
"ignore_msrs" kvm kernel parameter is set, then we should intercept
accesses to the BNDCFGS MSR, so that we can skip over the rdmsr/wrmsr
without raising a #GP fault.

Fixes: da8999d31818fdc8 ("KVM: x86: Intel MPX vmx and msr handle")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c6f4ad44aa95..763d27ee00fb 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -6547,7 +6547,6 @@ static __init int hardware_setup(void)
 	vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_CS, false);
 	vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
 	vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
-	vmx_disable_intercept_for_msr(MSR_IA32_BNDCFGS, true);
 
 	memcpy(vmx_msr_bitmap_legacy_x2apic_apicv,
 			vmx_msr_bitmap_legacy, PAGE_SIZE);
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 2/3] kvm: vmx: Guest BNDCFGS requires guest MPX support

[Jim Mattson]

The BNDCFGS MSR should only be exposed to the guest if the guest
supports MPX. (cf. the TSC_AUX MSR and RDTSCP.)

Fixes: 0dd376e709975779 ("KVM: x86: add MSR_IA32_BNDCFGS to msrs_to_save")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/vmx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 763d27ee00fb..846c60c74258 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3195,7 +3195,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = vmcs_readl(GUEST_SYSENTER_ESP);
 		break;
 	case MSR_IA32_BNDCFGS:
-		if (!kvm_mpx_supported())
+		if (!guest_cpuid_has_mpx(vcpu))
 			return 1;
 		msr_info->data = vmcs_read64(GUEST_BNDCFGS);
 		break;
@@ -3277,7 +3277,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		vmcs_writel(GUEST_SYSENTER_ESP, data);
 		break;
 	case MSR_IA32_BNDCFGS:
-		if (!kvm_mpx_supported())
+		if (!guest_cpuid_has_mpx(vcpu))
 			return 1;
 		vmcs_write64(GUEST_BNDCFGS, data);
 		break;
-- 
2.13.0.219.gdb65acc882-goog

[PATCH 3/3] kvm: vmx: Check value written to IA32_BNDCFGS

[Jim Mattson]

Bits 11:2 must be zero and the linear addess in bits 63:12 must be
canonical. Otherwise, WRMSR(BNDCFGS) should raise #GP.

Fixes: 0dd376e709975779 ("KVM: x86: add MSR_IA32_BNDCFGS to msrs_to_save")
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/include/asm/msr-index.h | 2 ++
 arch/x86/kvm/vmx.c               | 3 +++
 2 files changed, 5 insertions(+)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 673f9ac50f6d..dbf266b0d14a 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -420,6 +420,8 @@
 #define MSR_IA32_TSC_ADJUST             0x0000003b
 #define MSR_IA32_BNDCFGS		0x00000d90
 
+#define MSR_IA32_BNDCFGS_RSVD		0x00000ffc
+
 #define MSR_IA32_XSS			0x00000da0
 
 #define FEATURE_CONTROL_LOCKED				(1<<0)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 846c60c74258..04d428cd1d9d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3279,6 +3279,9 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	case MSR_IA32_BNDCFGS:
 		if (!guest_cpuid_has_mpx(vcpu))
 			return 1;
+		if (is_noncanonical_address(data & PAGE_MASK) ||
+		    (data & MSR_IA32_BNDCFGS_RSVD))
+			return 1;
 		vmcs_write64(GUEST_BNDCFGS, data);
 		break;
 	case MSR_IA32_TSC:
-- 
2.13.0.219.gdb65acc882-goog

Re: [PATCH 2/3] kvm: vmx: Guest BNDCFGS requires guest MPX support

[kbuild test robot]

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

Hi Jim,

[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v4.12-rc2 next-20170524]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jim-Mattson/kvm-vmx-Do-not-disable-intercepts-for-BNDCFGS/20170524-122859
base:   https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: x86_64-lkp (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   arch/x86/kvm/vmx.c: In function 'vmx_get_msr':
>> arch/x86/kvm/vmx.c:3198:8: error: implicit declaration of function 'guest_cpuid_has_mpx' [-Werror=implicit-function-declaration]
      if (!guest_cpuid_has_mpx(vcpu))
           ^~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/guest_cpuid_has_mpx +3198 arch/x86/kvm/vmx.c

  3192			msr_info->data = vmcs_readl(GUEST_SYSENTER_EIP);
  3193			break;
  3194		case MSR_IA32_SYSENTER_ESP:
  3195			msr_info->data = vmcs_readl(GUEST_SYSENTER_ESP);
  3196			break;
  3197		case MSR_IA32_BNDCFGS:
> 3198			if (!guest_cpuid_has_mpx(vcpu))
  3199				return 1;
  3200			msr_info->data = vmcs_read64(GUEST_BNDCFGS);
  3201			break;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25335 bytes --]

Re: [PATCH 1/3] kvm: vmx: Do not disable intercepts for BNDCFGS

[Radim Krčmář]

2017-05-23 11:52-0700, Jim Mattson:
> The MSR permission bitmaps are shared by all VMs. However, some VMs
> may not be configured to support MPX, even when the host does. If the
> host supports VMX and the guest does not, we should intercept accesses
> to the BNDCFGS MSR, so that we can synthesize a #GP
> fault. Furthermore, if the host does not support MPX and the
> "ignore_msrs" kvm kernel parameter is set, then we should intercept
> accesses to the BNDCFGS MSR, so that we can skip over the rdmsr/wrmsr
> without raising a #GP fault.
> 
> Fixes: da8999d31818fdc8 ("KVM: x86: Intel MPX vmx and msr handle")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---

IIUC, we don't even need faster accesses as BNDCFGS MSR should usually
be written once per boot and never read,

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

Re: [PATCH 3/3] kvm: vmx: Check value written to IA32_BNDCFGS

[Radim Krčmář]

2017-05-23 11:52-0700, Jim Mattson:
> Bits 11:2 must be zero and the linear addess in bits 63:12 must be
> canonical. Otherwise, WRMSR(BNDCFGS) should raise #GP.
> 
> Fixes: 0dd376e709975779 ("KVM: x86: add MSR_IA32_BNDCFGS to msrs_to_save")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

[PATCH 2/3 v2] kvm: x86: Guest BNDCFGS requires guest MPX support

[Jim Mattson]

The BNDCFGS MSR should only be exposed to the guest if the guest
supports MPX. (cf. the TSC_AUX MSR and RDTSCP.)

Fixes: 0dd376e709975779 ("KVM: x86: add MSR_IA32_BNDCFGS to msrs_to_save")
Change-Id: I3ad7c01bda616715137ceac878f3fa7e66b6b387
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.h | 8 ++++++++
 arch/x86/kvm/vmx.c   | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index a6fd40aade7c..da6728383052 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -144,6 +144,14 @@ static inline bool guest_cpuid_has_rtm(struct kvm_vcpu *vcpu)
 	return best && (best->ebx & bit(X86_FEATURE_RTM));
 }
 
+static inline bool guest_cpuid_has_mpx(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 7, 0);
+	return best && (best->ebx & bit(X86_FEATURE_MPX));
+}
+
 static inline bool guest_cpuid_has_rdtscp(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 763d27ee00fb..846c60c74258 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3195,7 +3195,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = vmcs_readl(GUEST_SYSENTER_ESP);
 		break;
 	case MSR_IA32_BNDCFGS:
-		if (!kvm_mpx_supported())
+		if (!guest_cpuid_has_mpx(vcpu))
 			return 1;
 		msr_info->data = vmcs_read64(GUEST_BNDCFGS);
 		break;
@@ -3277,7 +3277,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		vmcs_writel(GUEST_SYSENTER_ESP, data);
 		break;
 	case MSR_IA32_BNDCFGS:
-		if (!kvm_mpx_supported())
+		if (!guest_cpuid_has_mpx(vcpu))
 			return 1;
 		vmcs_write64(GUEST_BNDCFGS, data);
 		break;
-- 
2.13.0.219.gdb65acc882-goog

Re: [PATCH 2/3 v2] kvm: x86: Guest BNDCFGS requires guest MPX support

[Radim Krčmář]

2017-05-24 09:22-0700, Jim Mattson:
> The BNDCFGS MSR should only be exposed to the guest if the guest
> supports MPX. (cf. the TSC_AUX MSR and RDTSCP.)
> 
> Fixes: 0dd376e709975779 ("KVM: x86: add MSR_IA32_BNDCFGS to msrs_to_save")
> Change-Id: I3ad7c01bda616715137ceac878f3fa7e66b6b387
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> @@ -144,6 +144,14 @@ static inline bool guest_cpuid_has_rtm(struct kvm_vcpu *vcpu)
>  	return best && (best->ebx & bit(X86_FEATURE_RTM));
>  }
>  
> +static inline bool guest_cpuid_has_mpx(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +
> +	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> +	return best && (best->ebx & bit(X86_FEATURE_MPX));
> +}
> +
>  static inline bool guest_cpuid_has_rdtscp(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -3195,7 +3195,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  		msr_info->data = vmcs_readl(GUEST_SYSENTER_ESP);
>  		break;
>  	case MSR_IA32_BNDCFGS:
> -		if (!kvm_mpx_supported())
> +		if (!guest_cpuid_has_mpx(vcpu))
>  			return 1;
>  		msr_info->data = vmcs_read64(GUEST_BNDCFGS);

Userspace can force guest_cpuid_has_mpx() to return true even if the
host does not have MPX (GUEST_BNDCFGS in VMCS), which would allow it to
trigger vmread/vmwrite errors at will.

I think it would make most sense to fail KVM_SET_CPUID that tries to do
that, but checking for host support or silently clearing the bit still
seem better than the host error.

Thanks.

Re: [PATCH 2/3 v2] kvm: x86: Guest BNDCFGS requires guest MPX support

[Jim Mattson]

On Wed, May 24, 2017 at 10:28 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2017-05-24 09:22-0700, Jim Mattson:
>> The BNDCFGS MSR should only be exposed to the guest if the guest
>> supports MPX. (cf. the TSC_AUX MSR and RDTSCP.)
>>
>> Fixes: 0dd376e709975779 ("KVM: x86: add MSR_IA32_BNDCFGS to msrs_to_save")
>> Change-Id: I3ad7c01bda616715137ceac878f3fa7e66b6b387
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>> ---
>> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
>> @@ -144,6 +144,14 @@ static inline bool guest_cpuid_has_rtm(struct kvm_vcpu *vcpu)
>>       return best && (best->ebx & bit(X86_FEATURE_RTM));
>>  }
>>
>> +static inline bool guest_cpuid_has_mpx(struct kvm_vcpu *vcpu)
>> +{
>> +     struct kvm_cpuid_entry2 *best;
>> +
>> +     best = kvm_find_cpuid_entry(vcpu, 7, 0);
>> +     return best && (best->ebx & bit(X86_FEATURE_MPX));
>> +}
>> +
>>  static inline bool guest_cpuid_has_rdtscp(struct kvm_vcpu *vcpu)
>>  {
>>       struct kvm_cpuid_entry2 *best;
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> @@ -3195,7 +3195,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>               msr_info->data = vmcs_readl(GUEST_SYSENTER_ESP);
>>               break;
>>       case MSR_IA32_BNDCFGS:
>> -             if (!kvm_mpx_supported())
>> +             if (!guest_cpuid_has_mpx(vcpu))
>>                       return 1;
>>               msr_info->data = vmcs_read64(GUEST_BNDCFGS);
>
> Userspace can force guest_cpuid_has_mpx() to return true even if the
> host does not have MPX (GUEST_BNDCFGS in VMCS), which would allow it to
> trigger vmread/vmwrite errors at will.

Oops. I had wrongly assumed that the guest cpuid settings were validated.

> I think it would make most sense to fail KVM_SET_CPUID that tries to do
> that, but checking for host support or silently clearing the bit still
> seem better than the host error.

Guest cpuid settings should be validated, but I'm not going to bite
that off now. Let me just do both checks.

[PATCH 2/3 v3] kvm: x86: Guest BNDCFGS requires guest MPX support

[Jim Mattson]

The BNDCFGS MSR should only be exposed to the guest if the guest
supports MPX. (cf. the TSC_AUX MSR and RDTSCP.)

Fixes: 0dd376e709975779 ("KVM: x86: add MSR_IA32_BNDCFGS to msrs_to_save")
Change-Id: I3ad7c01bda616715137ceac878f3fa7e66b6b387
Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.h | 8 ++++++++
 arch/x86/kvm/vmx.c   | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index a6fd40aade7c..da6728383052 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -144,6 +144,14 @@ static inline bool guest_cpuid_has_rtm(struct kvm_vcpu *vcpu)
 	return best && (best->ebx & bit(X86_FEATURE_RTM));
 }
 
+static inline bool guest_cpuid_has_mpx(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 7, 0);
+	return best && (best->ebx & bit(X86_FEATURE_MPX));
+}
+
 static inline bool guest_cpuid_has_rdtscp(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 763d27ee00fb..c9bbbf509c2a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3195,7 +3195,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = vmcs_readl(GUEST_SYSENTER_ESP);
 		break;
 	case MSR_IA32_BNDCFGS:
-		if (!kvm_mpx_supported())
+		if (!kvm_mpx_supported() || !guest_cpuid_has_mpx(vcpu))
 			return 1;
 		msr_info->data = vmcs_read64(GUEST_BNDCFGS);
 		break;
@@ -3277,7 +3277,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		vmcs_writel(GUEST_SYSENTER_ESP, data);
 		break;
 	case MSR_IA32_BNDCFGS:
-		if (!kvm_mpx_supported())
+		if (!kvm_mpx_supported() || !guest_cpuid_has_mpx(vcpu))
 			return 1;
 		vmcs_write64(GUEST_BNDCFGS, data);
 		break;
-- 
2.13.0.219.gdb65acc882-goog

Re: [PATCH 2/3 v3] kvm: x86: Guest BNDCFGS requires guest MPX support

[Radim Krčmář]

2017-05-24 10:49-0700, Jim Mattson:
> The BNDCFGS MSR should only be exposed to the guest if the guest
> supports MPX. (cf. the TSC_AUX MSR and RDTSCP.)
> 
> Fixes: 0dd376e709975779 ("KVM: x86: add MSR_IA32_BNDCFGS to msrs_to_save")
> Change-Id: I3ad7c01bda616715137ceac878f3fa7e66b6b387
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---

Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

Re: [PATCH 1/3] kvm: vmx: Do not disable intercepts for BNDCFGS

[Radim Krčmář]

2017-05-23 11:52-0700, Jim Mattson:
> The MSR permission bitmaps are shared by all VMs. However, some VMs
> may not be configured to support MPX, even when the host does. If the
> host supports VMX and the guest does not, we should intercept accesses
> to the BNDCFGS MSR, so that we can synthesize a #GP
> fault. Furthermore, if the host does not support MPX and the
> "ignore_msrs" kvm kernel parameter is set, then we should intercept
> accesses to the BNDCFGS MSR, so that we can skip over the rdmsr/wrmsr
> without raising a #GP fault.
> 
> Fixes: da8999d31818fdc8 ("KVM: x86: Intel MPX vmx and msr handle")
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---

Queued all, thanks.