[PATCH v3 03/21] KVM: x86: Allow userspace to set KVM's max supported guest TSC frequency

[Sean Christopherson]

Reject KVM_SET_TSC_KHZ if the incoming frequency is strictly greater than
KVM's max supported frequency, not if the frequency is greater than *or*
equal to the max frequency.  The effective off-by-one bug came about via
(dubious) review feedback, which also subtly collided with a functional
change in later versions of the original TSC scaling series.

In v2 of the original TSC scaling series[1], KVM set its absolute min/max
to [1, UINT_MAX], i.e. allowed any value that would fit in the u32 passed
to KVM_SET_TSC_KHZ[2].

  min = max(1ULL,          __scale_tsc(tsc_khz, TSC_RATIO_MIN));
  max = min(0xffffffffULL, __scale_tsc(tsc_khz, TSC_RATIO_MAX));

That prompted Avi to suggest rejecting the "equals" case, presumably
because the check would always succeed given an absolute max of UINT_MAX?
But even that doesn't hold up to scrutiny, as allowing the actual min/max
isn't inherently unsafe.  Regardless, that feedback was taken and applied
to future versions, but only for the maximum, not the minimum.

  > +
  > +		r = -EINVAL;
  > +		if (user_tsc_khz<  kvm_min_guest_tsc_khz ||
  > +		    user_tsc_khz>  kvm_max_guest_tsc_khz)

  <= and >= are probably safer.

v3 of the series[3] then also realized that allowing UINT_MAX would lead to
undesirable interactions with KVM_GET_TSC_KHZ, which returns a *signed*
32-bit integer that is implicitly converted into a signed 64-bit value on
64-bit kernels.  I.e. allowing a value greater than INT_MAX would result
in userspace observing a negative value when doing KVM_GET_TSC_KHZ after
KVM_SET_TSC_KHZ.

 /*
  * Make sure the user can only configure tsc_khz values that
  * fit into a signed integer.
  * A min value is not calculated needed because it will always
  * be 1 on all machines and a value of 0 is used to disable
  * tsc-scaling for the vcpu.
  */
  max = min(0x7fffffffULL, __scale_tsc(tsc_khz, TSC_RATIO_MAX));

  kvm_max_guest_tsc_khz = max;

v3 also dropped the explicit minimum tracking, as both AMD and Intel
support a minimum *fractional* ratio of 1, i.e. AMD and Intel support a
minimum frequency of "host / 2^32" and "host / 2^48" respectively.  And
because KVM_GET_TSC_KHZ (and KVM itself) only supports frequencies that fit
in a signed 32-bit integer, even AMD's more coarse-grained ratio can scale
down any host frequency to '1', i.e. KVM can always support a minimum
frequency of 1KHz.  Rather than explicitly reject a frequency of 1KHz,
v3 simply dropped the minimum check, i.e. ignored the "<=" suggestion, but
kept the ">=" side of things.

As a result, KVM now has a bizarre uABI where KVM_SET_TSC_KHZ tops out at
INT_MAX-1 for no discernible reason.  Fix the off-by-one flaw to provide a
less weird uABI, and so that KVM_SET_TSC_KHZ accepts the maximum possible
value that can be returned by KVM_GET_TSC_KHZ (without running afoul of
casting issues; KVM doesn't actually sanity check that tsc_khz fits in a
signed 32-bit value, which is a non-issue in practice because the units are
KHz, not Hz, i.e. KVM_GET_TSC_KHZ is fine until CPUs with a TSC frequency
greater than ~2.147PHz come along).

Note, in practice, no real world VMM is likely to care.  As above, running
afoul of the off-by-one issue would mean trying to configure a virtual TSC
frequency that is three orders of magnitude greater than what current CPUs
support.

Opportunistically explain *why* KVM restricts KVM_SET_TSC_KHZ to values
that fit in a signed integer, as it requires far too much spelunking to
piece together the connection to KVM_GET_TSC_KHZ.

Link: https://lore.kernel.org/all/1300952424-32014-1-git-send-email-joerg.roedel@xxxxxxx [1]
Link: https://lore.kernel.org/all/1300952424-32014-7-git-send-email-joerg.roedel@xxxxxxx [2]
Link: https://lore.kernel.org/all/1301042691-22929-7-git-send-email-joerg.roedel@xxxxxxx [3]
Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
---
 arch/x86/kvm/x86.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 037c7ea5c5a7..1d25ffcf9273 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3718,7 +3718,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		user_tsc_khz = (u32)arg;
 
 		if (kvm_caps.has_tsc_control &&
-		    user_tsc_khz >= kvm_caps.max_guest_tsc_khz)
+		    user_tsc_khz > kvm_caps.max_guest_tsc_khz)
 			goto out;
 
 		if (user_tsc_khz == 0)
@@ -4685,7 +4685,7 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		user_tsc_khz = (u32)arg;
 
 		if (kvm_caps.has_tsc_control &&
-		    user_tsc_khz >= kvm_caps.max_guest_tsc_khz)
+		    user_tsc_khz > kvm_caps.max_guest_tsc_khz)
 			goto out;
 
 		if (user_tsc_khz == 0)
@@ -7174,7 +7174,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
 	if (kvm_caps.has_tsc_control) {
 		/*
 		 * Make sure the user can only configure tsc_khz values that
-		 * fit into a signed integer.
+		 * fit into a signed integer, otherwise KVM_GET_TSC_KHZ would
+		 * return a negative value and confuse userspace.
 		 * A min value is not calculated because it will always
 		 * be 1 on all machines.
 		 */
-- 
2.56.0.rc1.315.gc6ed9934b7-goog