Skip to content

Commit 17a2c62

Browse files
committed
KVM: nVMX: Check MSR load/store list counts during VM-Enter consistency checks
Explicitly verify the MSR load/store list counts are below the advertised limit as part of the initial consistency checks on the lists, so that code that consumes the count doesn't need to worry about extreme edge cases. Enforcing the limit during the initial checks fixes a flaw on 32-bit KVM where a sufficiently high @count could lead to overflow: arch/x86/kvm/vmx/nested.c:834 nested_vmx_check_msr_switch() warn: potential user controlled sizeof overflow 'addr + count * 16' '0-u64max + 16-68719476720' arch/x86/kvm/vmx/nested.c 827 static int nested_vmx_check_msr_switch(struct kvm_vcpu *vcpu, 828 u32 count, u64 addr) 829 { 830 if (count == 0) 831 return 0; 832 833 if (!kvm_vcpu_is_legal_aligned_gpa(vcpu, addr, 16) || --> 834 !kvm_vcpu_is_legal_gpa(vcpu, (addr + count * sizeof(struct vmx_msr_entry) - 1))) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ While the SDM doesn't explicitly state an illegal count results in VM-Fail, the SDM states that exceeding the limit may result in undefined behavior. I.e. the SDM gives hardware, and thus KVM, carte blanche to do literally anything in response to a count that exceeds the "recommended" limit. If the limit is exceeded, undefined processor behavior may result (including a machine check during the VMX transition). KVM already enforces the limit when processing the MSRs, i.e. already signals a late VM-Exit Consistency Check for VM-Enter, and generates a VMX Abort for VM-Exit. I.e. explicitly checking the limits simply means KVM will signal VM-Fail instead of VM-Exit or VMX Abort. Reported-by: Dan Carpenter <[email protected]> Closes: https://lore.kernel.org/all/[email protected] Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Sean Christopherson <[email protected]>
1 parent 45eb291 commit 17a2c62

File tree

1 file changed

+20
-11
lines changed

1 file changed

+20
-11
lines changed

arch/x86/kvm/vmx/nested.c

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -824,12 +824,30 @@ static int nested_vmx_check_apicv_controls(struct kvm_vcpu *vcpu,
824824
return 0;
825825
}
826826

827+
static u32 nested_vmx_max_atomic_switch_msrs(struct kvm_vcpu *vcpu)
828+
{
829+
struct vcpu_vmx *vmx = to_vmx(vcpu);
830+
u64 vmx_misc = vmx_control_msr(vmx->nested.msrs.misc_low,
831+
vmx->nested.msrs.misc_high);
832+
833+
return (vmx_misc_max_msr(vmx_misc) + 1) * VMX_MISC_MSR_LIST_MULTIPLIER;
834+
}
835+
827836
static int nested_vmx_check_msr_switch(struct kvm_vcpu *vcpu,
828837
u32 count, u64 addr)
829838
{
830839
if (count == 0)
831840
return 0;
832841

842+
/*
843+
* Exceeding the limit results in architecturally _undefined_ behavior,
844+
* i.e. KVM is allowed to do literally anything in response to a bad
845+
* limit. Immediately generate a consistency check so that code that
846+
* consumes the count doesn't need to worry about extreme edge cases.
847+
*/
848+
if (count > nested_vmx_max_atomic_switch_msrs(vcpu))
849+
return -EINVAL;
850+
833851
if (!kvm_vcpu_is_legal_aligned_gpa(vcpu, addr, 16) ||
834852
!kvm_vcpu_is_legal_gpa(vcpu, (addr + count * sizeof(struct vmx_msr_entry) - 1)))
835853
return -EINVAL;
@@ -940,15 +958,6 @@ static int nested_vmx_store_msr_check(struct kvm_vcpu *vcpu,
940958
return 0;
941959
}
942960

943-
static u32 nested_vmx_max_atomic_switch_msrs(struct kvm_vcpu *vcpu)
944-
{
945-
struct vcpu_vmx *vmx = to_vmx(vcpu);
946-
u64 vmx_misc = vmx_control_msr(vmx->nested.msrs.misc_low,
947-
vmx->nested.msrs.misc_high);
948-
949-
return (vmx_misc_max_msr(vmx_misc) + 1) * VMX_MISC_MSR_LIST_MULTIPLIER;
950-
}
951-
952961
/*
953962
* Load guest's/host's msr at nested entry/exit.
954963
* return 0 for success, entry index for failure.
@@ -965,7 +974,7 @@ static u32 nested_vmx_load_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
965974
u32 max_msr_list_size = nested_vmx_max_atomic_switch_msrs(vcpu);
966975

967976
for (i = 0; i < count; i++) {
968-
if (unlikely(i >= max_msr_list_size))
977+
if (WARN_ON_ONCE(i >= max_msr_list_size))
969978
goto fail;
970979

971980
if (kvm_vcpu_read_guest(vcpu, gpa + i * sizeof(e),
@@ -1053,7 +1062,7 @@ static int nested_vmx_store_msr(struct kvm_vcpu *vcpu, u64 gpa, u32 count)
10531062
u32 max_msr_list_size = nested_vmx_max_atomic_switch_msrs(vcpu);
10541063

10551064
for (i = 0; i < count; i++) {
1056-
if (unlikely(i >= max_msr_list_size))
1065+
if (WARN_ON_ONCE(i >= max_msr_list_size))
10571066
return -EINVAL;
10581067

10591068
if (!read_and_check_msr_entry(vcpu, gpa, i, &e))

0 commit comments

Comments
 (0)