* [PATCH 1/4] KVM: x86: Don't let userspace set host-reserved cr4 bits
2019-12-10 22:44 [PATCH 0/4] KVM: x86: Add checks on host-reserved cr4 bits Sean Christopherson
@ 2019-12-10 22:44 ` Sean Christopherson
2019-12-10 22:44 ` [PATCH 2/4] KVM: x86: Ensure all logical CPUs have consistent reserved " Sean Christopherson
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2019-12-10 22:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, Jun Nakajima
Calculate the host-reserved cr4 bits at runtime based on the system's
capabilities (using logic similar to __do_cpuid_func()), and use the
dynamically generated mask for the reserved bit check in kvm_set_cr4()
instead using of the static CR4_RESERVED_BITS define. This prevents
userspace from "enabling" features in cr4 that are not supported by the
system, e.g. by ignoring KVM_GET_SUPPORTED_CPUID and specifying a bogus
CPUID for the vCPU.
Allowing userspace to set unsupported bits in cr4 can lead to a variety
of undesirable behavior, e.g. failed VM-Enter, and in general increases
KVM's attack surface. A crafty userspace can even abuse CR4.LA57 to
induce an unchecked #GP on a WRMSR.
On a platform without LA57 support:
KVM_SET_CPUID2 // CPUID_7_0_ECX.LA57 = 1
KVM_SET_SREGS // CR4.LA57 = 1
KVM_SET_MSRS // KERNEL_GS_BASE = 0x0004000000000000
KVM_RUN
leads to a #GP when writing KERNEL_GS_BASE into hardware:
unchecked MSR access error: WRMSR to 0xc0000102 (tried to write 0x0004000000000000)
at rIP: 0xffffffffa00f239a (vmx_prepare_switch_to_guest+0x10a/0x1d0 [kvm_intel])
Call Trace:
kvm_arch_vcpu_ioctl_run+0x671/0x1c70 [kvm]
kvm_vcpu_ioctl+0x36b/0x5d0 [kvm]
do_vfs_ioctl+0xa1/0x620
ksys_ioctl+0x66/0x70
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x4c/0x170
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7fc08133bf47
Note, the above sequence fails VM-Enter due to invalid guest state.
Userspace can allow VM-Enter to succeed (after the WRMSR #GP) by adding
a KVM_SET_SREGS w/ CR4.LA57=0 after KVM_SET_MSRS, in which case KVM will
technically leak the host's KERNEL_GS_BASE into the guest. But, as
KERNEL_GS_BASE is a userspace-defined value/address, the leak is largely
benign as a malicious userspace would simply be exposing its own data to
the guest, and attacking a benevolent userspace would require multiple
bugs in the userspace VMM.
Cc: stable@vger.kernel.org
Cc: Jun Nakajima <jun.nakajima@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/x86.c | 35 ++++++++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8bb2fb1705ff..321eecb4cffd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -92,6 +92,8 @@ u64 __read_mostly efer_reserved_bits = ~((u64)(EFER_SCE | EFER_LME | EFER_LMA));
static u64 __read_mostly efer_reserved_bits = ~((u64)EFER_SCE);
#endif
+static u64 __read_mostly cr4_reserved_bits = CR4_RESERVED_BITS;
+
#define VM_STAT(x, ...) offsetof(struct kvm, stat.x), KVM_STAT_VM, ## __VA_ARGS__
#define VCPU_STAT(x, ...) offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU, ## __VA_ARGS__
@@ -878,9 +880,38 @@ int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
}
EXPORT_SYMBOL_GPL(kvm_set_xcr);
+static u64 kvm_host_cr4_reserved_bits(struct cpuinfo_x86 *c)
+{
+ u64 reserved_bits = CR4_RESERVED_BITS;
+
+ if (!cpu_has(c, X86_FEATURE_XSAVE))
+ reserved_bits |= X86_CR4_OSXSAVE;
+
+ if (!cpu_has(c, X86_FEATURE_SMEP))
+ reserved_bits |= X86_CR4_SMEP;
+
+ if (!cpu_has(c, X86_FEATURE_SMAP))
+ reserved_bits |= X86_CR4_SMAP;
+
+ if (!cpu_has(c, X86_FEATURE_FSGSBASE))
+ reserved_bits |= X86_CR4_FSGSBASE;
+
+ if (!cpu_has(c, X86_FEATURE_PKU))
+ reserved_bits |= X86_CR4_PKE;
+
+ if (!cpu_has(c, X86_FEATURE_LA57) &&
+ !(cpuid_ecx(0x7) & bit(X86_FEATURE_LA57)))
+ reserved_bits |= X86_CR4_LA57;
+
+ if (!cpu_has(c, X86_FEATURE_UMIP) && !kvm_x86_ops->umip_emulated())
+ reserved_bits |= X86_CR4_UMIP;
+
+ return reserved_bits;
+}
+
static int kvm_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
{
- if (cr4 & CR4_RESERVED_BITS)
+ if (cr4 & cr4_reserved_bits)
return -EINVAL;
if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) && (cr4 & X86_CR4_OSXSAVE))
@@ -9354,6 +9385,8 @@ int kvm_arch_hardware_setup(void)
if (r != 0)
return r;
+ cr4_reserved_bits = kvm_host_cr4_reserved_bits(&boot_cpu_data);
+
if (kvm_has_tsc_control) {
/*
* Make sure the user can only configure tsc_khz values that
--
2.24.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/4] KVM: x86: Ensure all logical CPUs have consistent reserved cr4 bits
2019-12-10 22:44 [PATCH 0/4] KVM: x86: Add checks on host-reserved cr4 bits Sean Christopherson
2019-12-10 22:44 ` [PATCH 1/4] KVM: x86: Don't let userspace set " Sean Christopherson
@ 2019-12-10 22:44 ` Sean Christopherson
2019-12-10 22:44 ` [PATCH 3/4] KVM: x86: Drop special XSAVE handling from guest_cpuid_has() Sean Christopherson
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2019-12-10 22:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, Jun Nakajima
Check the current CPU's reserved cr4 bits against the mask calculated
for the boot CPU to ensure consistent behavior across all CPUs.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
I'm on the fence as to whether this is gratuitous or useful, which is
why it's a separate patch and not tagged for stable.
arch/x86/kvm/x86.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 321eecb4cffd..ab3a4104febf 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9415,6 +9415,13 @@ void kvm_arch_hardware_unsetup(void)
int kvm_arch_check_processor_compat(void)
{
+ struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
+
+ WARN_ON(!irqs_disabled());
+
+ if (kvm_host_cr4_reserved_bits(c) != cr4_reserved_bits)
+ return -EIO;
+
return kvm_x86_ops->check_processor_compatibility();
}
--
2.24.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/4] KVM: x86: Drop special XSAVE handling from guest_cpuid_has()
2019-12-10 22:44 [PATCH 0/4] KVM: x86: Add checks on host-reserved cr4 bits Sean Christopherson
2019-12-10 22:44 ` [PATCH 1/4] KVM: x86: Don't let userspace set " Sean Christopherson
2019-12-10 22:44 ` [PATCH 2/4] KVM: x86: Ensure all logical CPUs have consistent reserved " Sean Christopherson
@ 2019-12-10 22:44 ` Sean Christopherson
2019-12-10 22:44 ` [PATCH 4/4] KVM: x86: Add macro to ensure reserved cr4 bits checks stay in sync Sean Christopherson
2020-01-15 18:05 ` [PATCH 0/4] KVM: x86: Add checks on host-reserved cr4 bits Paolo Bonzini
4 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2019-12-10 22:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, Jun Nakajima
Now that KVM prevents setting host-reserved CR4 bits, drop the dedicated
XSAVE check in guest_cpuid_has() in favor of open coding similar checks
in the SVM/VMX XSAVES enabling flows.
Note, checking boot_cpu_has(X86_FEATURE_XSAVE) in the XSAVES flows is
technically redundant with respect to the CR4 reserved bit checks, e.g.
XSAVES #UDs if CR4.OSXSAVE=0 and arch.xsaves_enabled is consumed if and
only if CR4.OXSAVE=1 in guest. Keep (add?) the explicit boot_cpu_has()
checks to help document KVM's usage of arch.xsaves_enabled.
No functional change intended.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/cpuid.h | 4 ----
arch/x86/kvm/svm.c | 1 +
arch/x86/kvm/vmx/vmx.c | 1 +
3 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index d78a61408243..b82ae4d3dc71 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -93,10 +93,6 @@ static __always_inline bool guest_cpuid_has(struct kvm_vcpu *vcpu, unsigned x86_
{
int *reg;
- if (x86_feature == X86_FEATURE_XSAVE &&
- !static_cpu_has(X86_FEATURE_XSAVE))
- return false;
-
reg = guest_cpuid_get_register(vcpu, x86_feature);
if (!reg)
return false;
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 8f1b715dfde8..ba23f18b2eee 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -5913,6 +5913,7 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
struct vcpu_svm *svm = to_svm(vcpu);
vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
+ boot_cpu_has(X86_FEATURE_XSAVE) &&
boot_cpu_has(X86_FEATURE_XSAVES);
/* Update nrips enabled cache */
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 51e3b27f90ed..74c6e70e5863 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -4048,6 +4048,7 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
if (vmx_xsaves_supported()) {
/* Exposing XSAVES only when XSAVE is exposed */
bool xsaves_enabled =
+ boot_cpu_has(X86_FEATURE_XSAVE) &&
guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
--
2.24.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/4] KVM: x86: Add macro to ensure reserved cr4 bits checks stay in sync
2019-12-10 22:44 [PATCH 0/4] KVM: x86: Add checks on host-reserved cr4 bits Sean Christopherson
` (2 preceding siblings ...)
2019-12-10 22:44 ` [PATCH 3/4] KVM: x86: Drop special XSAVE handling from guest_cpuid_has() Sean Christopherson
@ 2019-12-10 22:44 ` Sean Christopherson
2020-01-15 18:05 ` [PATCH 0/4] KVM: x86: Add checks on host-reserved cr4 bits Paolo Bonzini
4 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2019-12-10 22:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Jim Mattson,
Joerg Roedel, kvm, linux-kernel, Jun Nakajima
Add a helper macro to generate the set of reserved cr4 bits for both
host and guest to ensure that adding a check on guest capabilities is
also added for host capabilities, and vice versa.
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
arch/x86/kvm/x86.c | 65 ++++++++++++++++++----------------------------
1 file changed, 25 insertions(+), 40 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ab3a4104febf..d2ab4da75783 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -880,31 +880,34 @@ int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
}
EXPORT_SYMBOL_GPL(kvm_set_xcr);
+#define __cr4_reserved_bits(__cpu_has, __c) \
+({ \
+ u64 __reserved_bits = CR4_RESERVED_BITS; \
+ \
+ if (!__cpu_has(__c, X86_FEATURE_XSAVE)) \
+ __reserved_bits |= X86_CR4_OSXSAVE; \
+ if (!__cpu_has(__c, X86_FEATURE_SMEP)) \
+ __reserved_bits |= X86_CR4_SMEP; \
+ if (!__cpu_has(__c, X86_FEATURE_SMAP)) \
+ __reserved_bits |= X86_CR4_SMAP; \
+ if (!__cpu_has(__c, X86_FEATURE_FSGSBASE)) \
+ __reserved_bits |= X86_CR4_FSGSBASE; \
+ if (!__cpu_has(__c, X86_FEATURE_PKU)) \
+ __reserved_bits |= X86_CR4_PKE; \
+ if (!__cpu_has(__c, X86_FEATURE_LA57)) \
+ __reserved_bits |= X86_CR4_LA57; \
+ __reserved_bits; \
+})
+
static u64 kvm_host_cr4_reserved_bits(struct cpuinfo_x86 *c)
{
- u64 reserved_bits = CR4_RESERVED_BITS;
+ u64 reserved_bits = __cr4_reserved_bits(cpu_has, c);
- if (!cpu_has(c, X86_FEATURE_XSAVE))
- reserved_bits |= X86_CR4_OSXSAVE;
+ if (cpuid_ecx(0x7) & bit(X86_FEATURE_LA57))
+ reserved_bits &= ~X86_CR4_LA57;
- if (!cpu_has(c, X86_FEATURE_SMEP))
- reserved_bits |= X86_CR4_SMEP;
-
- if (!cpu_has(c, X86_FEATURE_SMAP))
- reserved_bits |= X86_CR4_SMAP;
-
- if (!cpu_has(c, X86_FEATURE_FSGSBASE))
- reserved_bits |= X86_CR4_FSGSBASE;
-
- if (!cpu_has(c, X86_FEATURE_PKU))
- reserved_bits |= X86_CR4_PKE;
-
- if (!cpu_has(c, X86_FEATURE_LA57) &&
- !(cpuid_ecx(0x7) & bit(X86_FEATURE_LA57)))
- reserved_bits |= X86_CR4_LA57;
-
- if (!cpu_has(c, X86_FEATURE_UMIP) && !kvm_x86_ops->umip_emulated())
- reserved_bits |= X86_CR4_UMIP;
+ if (kvm_x86_ops->umip_emulated())
+ reserved_bits &= ~X86_CR4_UMIP;
return reserved_bits;
}
@@ -914,25 +917,7 @@ static int kvm_valid_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
if (cr4 & cr4_reserved_bits)
return -EINVAL;
- if (!guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) && (cr4 & X86_CR4_OSXSAVE))
- return -EINVAL;
-
- if (!guest_cpuid_has(vcpu, X86_FEATURE_SMEP) && (cr4 & X86_CR4_SMEP))
- return -EINVAL;
-
- if (!guest_cpuid_has(vcpu, X86_FEATURE_SMAP) && (cr4 & X86_CR4_SMAP))
- return -EINVAL;
-
- if (!guest_cpuid_has(vcpu, X86_FEATURE_FSGSBASE) && (cr4 & X86_CR4_FSGSBASE))
- return -EINVAL;
-
- if (!guest_cpuid_has(vcpu, X86_FEATURE_PKU) && (cr4 & X86_CR4_PKE))
- return -EINVAL;
-
- if (!guest_cpuid_has(vcpu, X86_FEATURE_LA57) && (cr4 & X86_CR4_LA57))
- return -EINVAL;
-
- if (!guest_cpuid_has(vcpu, X86_FEATURE_UMIP) && (cr4 & X86_CR4_UMIP))
+ if (cr4 & __cr4_reserved_bits(guest_cpuid_has, vcpu))
return -EINVAL;
return 0;
--
2.24.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/4] KVM: x86: Add checks on host-reserved cr4 bits
2019-12-10 22:44 [PATCH 0/4] KVM: x86: Add checks on host-reserved cr4 bits Sean Christopherson
` (3 preceding siblings ...)
2019-12-10 22:44 ` [PATCH 4/4] KVM: x86: Add macro to ensure reserved cr4 bits checks stay in sync Sean Christopherson
@ 2020-01-15 18:05 ` Paolo Bonzini
4 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-01-15 18:05 UTC (permalink / raw)
To: Sean Christopherson
Cc: Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel, kvm,
linux-kernel, Jun Nakajima
On 10/12/19 23:44, Sean Christopherson wrote:
> KVM currently doesn't incorporate the platform's capabilities in its cr4
> reserved bit checks and instead checks only whether KVM is aware of the
> feature in general and whether or not userspace has advertised the feature
> to the guest.
>
> Lack of checking allows userspace/guest to set unsupported bits in cr4.
> For the most part, setting unsupported bits will simply cause VM-Enter to
> fail. The one existing exception is OSXSAVE, which is conditioned on host
> support as checking only guest_cpuid_has() would result in KVM attempting
> XSAVE, leading to faults and WARNs.
>
> 57-bit virtual addressing has introduced another case where setting an
> unsupported bit (cr4.LA57) can induce a fault in the host. In the LA57
> case, userspace can set the guest's cr4.LA57 by advertising LA57 support
> via CPUID and abuse the bogus cr4.LA57 to effectively bypass KVM's
> non-canonical address check, ultimately causing a #GP when VMX writes
> the guest's bogus address to MSR_KERNEL_GS_BASE during VM-Enter.
>
> Given that the best case scenario is a failed VM-Enter, there's no sane
> reason to allow setting unsupported bits in cr4. Fix the LA57 bug by not
> allowing userspace or the guest to set cr4 bits that are not supported
> by the platform.
>
> Sean Christopherson (4):
> KVM: x86: Don't let userspace set host-reserved cr4 bits
> KVM: x86: Ensure all logical CPUs have consistent reserved cr4 bits
> KVM: x86: Drop special XSAVE handling from guest_cpuid_has()
> KVM: x86: Add macro to ensure reserved cr4 bits checks stay in sync
>
> arch/x86/kvm/cpuid.h | 4 ---
> arch/x86/kvm/svm.c | 1 +
> arch/x86/kvm/vmx/vmx.c | 1 +
> arch/x86/kvm/x86.c | 65 +++++++++++++++++++++++++++++-------------
> 4 files changed, 47 insertions(+), 24 deletions(-)
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread