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

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Thu, Jun 06, 2024, Mathias Krause wrote:
> If, on a 64 bit system, a vCPU ID is provided that has the upper 32 bits
> set to a non-zero value, it may get accepted if the truncated to 32 bits
> integer value is below KVM_MAX_VCPU_IDS and 'max_vcpus'. This feels very
> wrong and triggered the reporting logic of PaX's SIZE_OVERFLOW plugin.
> 
> Instead of silently truncating and accepting such values, pass the full
> value to kvm_vm_ioctl_create_vcpu() and make the existing limit checks
> return an error.
> 
> Even if this is a userland ABI breaking change, no sane userland could
> have ever relied on that behaviour.
> 
> Reported-by: PaX's SIZE_OVERFLOW plugin running on grsecurity's syzkaller
> Fixes: 6aa8b732ca01 ("[PATCH] kvm: userspace interface")
> Cc: Emese Revfy <re.emese@xxxxxxxxx>
> Cc: PaX Team <pageexec@xxxxxxxxxxx>
> Signed-off-by: Mathias Krause <minipli@xxxxxxxxxxxxxx>
> ---
>  virt/kvm/kvm_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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.

If we really care enough to fix this, my vote is for something like so:

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4965196cad58..08adfdb2817e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4200,13 +4200,14 @@ 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;
+       u32 id = __id;
 
-       if (id >= KVM_MAX_VCPU_IDS)
+       if (id != __id || id >= KVM_MAX_VCPU_IDS)
                return -EINVAL;
 
        mutex_lock(&kvm->lock);