Re: [PATCH 1/2] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU

[Sean Christopherson <seanjc@google.com>]

On Thu, Jun 06, 2024, Mathias Krause wrote:
> On 06.06.24 00:31, Sean Christopherson wrote:
> > On Thu, Jun 06, 2024, Mathias Krause wrote:
> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >> index 14841acb8b95..9f18fc42f018 100644
> >> --- a/virt/kvm/kvm_main.c
> >> +++ b/virt/kvm/kvm_main.c
> >> @@ -4200,7 +4200,7 @@ static void kvm_create_vcpu_debugfs(struct kvm_vcpu *vcpu)
> >>  /*
> >>   * Creates some virtual cpus.  Good luck creating more than one.
> >>   */
> >> -static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> >> +static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
> > 
> > Hmm, I don't love that KVM subtly relies on the KVM_MAX_VCPU_IDS check to guard
> > against truncation when passing @id to kvm_arch_vcpu_precreate(), kvm_vcpu_init(),
> > etc.  I doubt that it will ever be problematic, but it _looks_ like a bug.
> 
> It's not subtle but very explicit. KVM_MAX_VCPU_IDS is a small positive
> number, depending on some arch specific #define, but with x86 allowing
> for the largest value of 4 * 4096. That value, for sure, cannot exceed
> U32_MAX, so an explicit truncation isn't needed as the upper bits will
> already be zero if the limit check passes.
> 
> While subtile integer truncation is the bug that my patch is actually
> fixing, it is for the *userland* facing part of it, as in clarifying the
> ABI to work on "machine-sized words", i.e. a ulong, and doing the limit
> checks on these.
> 
> *In-kernel* APIs truncate / sign extend / mix signed/unsigned values all
> the time. The kernel is full of these. Trying to "fix" them all is an
> uphill battle not worth fighting, imho.

Oh, I'm not worry about something going wrong with the actual truncation.

What I don't like is the primary in-kernal API, kvm_vm_ioctl_create_vcpu(), taking
an unsigned long, but everything underneath converting that to an unsigned int,
without much of anything to give the reader a clue that the truncation is
deliberate.  

Similarly, without the context of the changelog, it's not at all obvious why
kvm_vm_ioctl_create_vcpu() takes an unsigned long.

E.g. x86 has another potentially more restrictive check on @id, and it looks
quite odd to check @id against KVM_MAX_VCPU_IDS as an "unsigned long" in flow
flow, but as an "unsigned int" in another.

int kvm_arch_vcpu_precreate(struct kvm *kvm, unsigned int id)
{
	if (kvm_check_tsc_unstable() && kvm->created_vcpus)
		pr_warn_once("SMP vm created on host with unstable TSC; "
			     "guest TSC will not be reliable\n");

	if (!kvm->arch.max_vcpu_ids)
		kvm->arch.max_vcpu_ids = KVM_MAX_VCPU_IDS;

	if (id >= kvm->arch.max_vcpu_ids)
		return -EINVAL;

> I'd rather suggest to add a build time assert instead, as the existing
> runtime check is sufficient (with my u32->ulong change). Something like
> this:
> 
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4200,12 +4200,13 @@ static void kvm_create_vcpu_debugfs(struct
> kvm_vcpu *vcpu)
>  /*
>   * Creates some virtual cpus.  Good luck creating more than one.
>   */
> -static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> +static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
>  {
>         int r;
>         struct kvm_vcpu *vcpu;
>         struct page *page;
> 
> +       BUILD_BUG_ON(KVM_MAX_VCPU_IDS > INT_MAX);

This should be UINT_MAX, no?  Regardless, the "need" for an explicit BUILD_BUG_ON()
is another reason I dislike relying on the KVM_MAX_VCPU_IDS check to detect
truncation.  If @id is checked as a 32-bit value, and we somehow screw up and
define KVM_MAX_VCPU_IDS to be a 64-bit value, clang will rightly complain that
the check is useless, e.g. given "#define KVM_MAX_VCPU_ID_TEST	BIT(32)"

arch/x86/kvm/x86.c:12171:9: error: result of comparison of constant 4294967296 with
expression of type 'unsigned int' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
        if (id > KVM_MAX_VCPU_ID_TEST)
            ~~ ^ ~~~~~~~~~~~~~~~~~~~~
1 error generated.


>         if (id >= KVM_MAX_VCPU_IDS)
>                 return -EINVAL;

What if we do an explicit check before calling kvm_vm_ioctl_create_vcpu()?  That
would avoid the weird __id param, and provide a convenient location to document
exactly why KVM checks for truncation.

We could also move the "if (id >= KVM_MAX_VCPU_IDS)" check to kvm_vm_ioctl(),
but I don't love that, because again IMO it makes the code less readable overall,
loses clang's tuautological constant check, and the cost of the extra check against
UINT_MAX is completely negligible.  

Though if I had to choose, I'd prefer moving the check to kvm_vm_ioctl() over
taking an "unsigned long" in kvm_vm_ioctl_create_vcpu().

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4965196cad58..8155146b16cd 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -5083,6 +5083,13 @@ static long kvm_vm_ioctl(struct file *filp,
                return -EIO;
        switch (ioctl) {
        case KVM_CREATE_VCPU:
+               /*
+                * KVM tracks vCPU ID as a 32-bit value, be kind to userspace
+                * and reject too-large values instead of silently truncating.
+                */
+               if (arg > UINT_MAX)
+                       return -EINVAL;
+
                r = kvm_vm_ioctl_create_vcpu(kvm, arg);
                break;
        case KVM_ENABLE_CAP: {