[PATCH v2 0/4] KVM: Reject vCPU IDs above 2^32

[PATCH v2 0/4] KVM: Reject vCPU IDs above 2^32

[Mathias Krause]

vCPU IDs above 2^32 are currently not rejected as invalid for
KVM_CREATE_VCPU and KVM_SET_BOOT_CPU_ID.

Below patches fix this and add selftests for it.

Please apply!

Thanks,
Mathias

v1: https://lore.kernel.org/kvm/20240605220504.2941958-1-minipli@grsecurity.net/

changes v1->v2:
- add comment and build bug to make truncation check more obvious (Sean)
- handle KVM_SET_BOOT_CPU_ID similar

Mathias Krause (4):
  KVM: Reject overly excessive IDs in KVM_CREATE_VCPU
  KVM: selftests: Test vCPU IDs above 2^32
  KVM: Limit check IDs for KVM_SET_BOOT_CPU_ID
  KVM: selftests: Test vCPU boot IDs above 2^32

 arch/x86/kvm/x86.c                                   | 12 +++++++++---
 .../selftests/kvm/x86_64/max_vcpuid_cap_test.c       | 11 ++++++++++-
 tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c | 11 +++++++++++
 virt/kvm/kvm_main.c                                  | 10 +++++++++-
 4 files changed, 39 insertions(+), 5 deletions(-)

-- 
2.30.2

[PATCH v2 1/4] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU

[Mathias Krause]

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@gmail.com>
Cc: PaX Team <pageexec@freemail.hu>
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
v2:
- add comment and build bug to make truncation check more obvious (Sean)
 virt/kvm/kvm_main.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 14841acb8b95..b04e87f6568f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4200,12 +4200,20 @@ 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;
 
+	/*
+	 * KVM tracks vCPU IDs as 'int', be kind to userspace and reject
+	 * too-large values instead of silently truncating.
+	 *
+	 * Also ensure we're not breaking this assumption by accidentally
+	 * pushing KVM_MAX_VCPU_IDS above INT_MAX.
+	 */
+	BUILD_BUG_ON(KVM_MAX_VCPU_IDS > INT_MAX);
 	if (id >= KVM_MAX_VCPU_IDS)
 		return -EINVAL;
 
-- 
2.30.2

[PATCH v2 2/4] KVM: selftests: Test vCPU IDs above 2^32

[Mathias Krause]

The KVM_CREATE_VCPU ioctl ABI had an implicit integer truncation bug,
allowing 2^32 aliases for a vCPU ID by setting the upper 32 bits of a 64
bit ioctl() argument.

Verify this no longer works and gets rejected with an error.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 .../selftests/kvm/x86_64/max_vcpuid_cap_test.c        | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kvm/x86_64/max_vcpuid_cap_test.c b/tools/testing/selftests/kvm/x86_64/max_vcpuid_cap_test.c
index 3cc4b86832fe..92cb2f4aef6d 100644
--- a/tools/testing/selftests/kvm/x86_64/max_vcpuid_cap_test.c
+++ b/tools/testing/selftests/kvm/x86_64/max_vcpuid_cap_test.c
@@ -35,10 +35,19 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(ret < 0,
 		    "Setting KVM_CAP_MAX_VCPU_ID multiple times should fail");
 
-	/* Create vCPU with id beyond KVM_CAP_MAX_VCPU_ID cap*/
+	/* Create vCPU with id beyond KVM_CAP_MAX_VCPU_ID cap */
 	ret = __vm_ioctl(vm, KVM_CREATE_VCPU, (void *)MAX_VCPU_ID);
 	TEST_ASSERT(ret < 0, "Creating vCPU with ID > MAX_VCPU_ID should fail");
 
+	/* Create vCPU with id beyond UINT_MAX */
+	ret = __vm_ioctl(vm, KVM_CREATE_VCPU, (void *)(1L << 32));
+	TEST_ASSERT(ret < 0, "Creating vCPU with ID > UINT_MAX should fail");
+
+	/* Create vCPU with id within bounds */
+	ret = __vm_ioctl(vm, KVM_CREATE_VCPU, (void *)0);
+	TEST_ASSERT(ret >= 0, "Creating vCPU with ID 0 should succeed");
+
+	close(ret);
 	kvm_vm_free(vm);
 	return 0;
 }
-- 
2.30.2

[PATCH v2 3/4] KVM: Limit check IDs for KVM_SET_BOOT_CPU_ID

[Mathias Krause]

Do not accept IDs which are definitely invalid by limit checking the
passed value against KVM_MAX_VCPU_IDS.

This ensures invalid values, especially on 64-bit systems, don't go
unnoticed and lead to a valid id by chance when truncated by the final
assignment.

Fixes: 73880c80aa9c ("KVM: Break dependency between vcpu index in vcpus array and vcpu_id.")
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 arch/x86/kvm/x86.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 082ac6d95a3a..8bc7b8b2dfc5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7220,10 +7220,16 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 	case KVM_SET_BOOT_CPU_ID:
 		r = 0;
 		mutex_lock(&kvm->lock);
-		if (kvm->created_vcpus)
+		if (kvm->created_vcpus) {
 			r = -EBUSY;
-		else
-			kvm->arch.bsp_vcpu_id = arg;
+			goto set_boot_cpu_id_out;
+		}
+		if (arg > KVM_MAX_VCPU_IDS) {
+			r = -EINVAL;
+			goto set_boot_cpu_id_out;
+		}
+		kvm->arch.bsp_vcpu_id = arg;
+set_boot_cpu_id_out:
 		mutex_unlock(&kvm->lock);
 		break;
 #ifdef CONFIG_KVM_XEN
-- 
2.30.2

[PATCH v2 4/4] KVM: selftests: Test vCPU boot IDs above 2^32

[Mathias Krause]

The KVM_SET_BOOT_CPU_ID ioctl missed to reject invalid vCPU IDs. Verify
this no longer works and gets rejected with an appropriate error code.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
index d691d86e5bc3..50a0c3f61baf 100644
--- a/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
+++ b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
@@ -33,6 +33,13 @@ static void guest_not_bsp_vcpu(void *arg)
 	GUEST_DONE();
 }
 
+static void test_set_invalid_bsp(struct kvm_vm *vm)
+{
+	int r = __vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *)(1L << 32));
+
+	TEST_ASSERT(r == -1 && errno == EINVAL, "invalid KVM_SET_BOOT_CPU_ID set");
+}
+
 static void test_set_bsp_busy(struct kvm_vcpu *vcpu, const char *msg)
 {
 	int r = __vm_ioctl(vcpu->vm, KVM_SET_BOOT_CPU_ID,
@@ -75,11 +82,15 @@ static void run_vcpu(struct kvm_vcpu *vcpu)
 static struct kvm_vm *create_vm(uint32_t nr_vcpus, uint32_t bsp_vcpu_id,
 				struct kvm_vcpu *vcpus[])
 {
+	static int invalid_bsp_tested;
 	struct kvm_vm *vm;
 	uint32_t i;
 
 	vm = vm_create(nr_vcpus);
 
+	if (!invalid_bsp_tested++)
+		test_set_invalid_bsp(vm);
+
 	vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *)(unsigned long)bsp_vcpu_id);
 
 	for (i = 0; i < nr_vcpus; i++)
-- 
2.30.2

Re: [PATCH v2 3/4] KVM: Limit check IDs for KVM_SET_BOOT_CPU_ID

[Sean Christopherson]

On Wed, Jun 12, 2024, Mathias Krause wrote:
> Do not accept IDs which are definitely invalid by limit checking the
> passed value against KVM_MAX_VCPU_IDS.
> 
> This ensures invalid values, especially on 64-bit systems, don't go
> unnoticed and lead to a valid id by chance when truncated by the final
> assignment.
> 
> Fixes: 73880c80aa9c ("KVM: Break dependency between vcpu index in vcpus array and vcpu_id.")
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---
>  arch/x86/kvm/x86.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 082ac6d95a3a..8bc7b8b2dfc5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7220,10 +7220,16 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>  	case KVM_SET_BOOT_CPU_ID:
>  		r = 0;
>  		mutex_lock(&kvm->lock);
> -		if (kvm->created_vcpus)
> +		if (kvm->created_vcpus) {
>  			r = -EBUSY;
> -		else
> -			kvm->arch.bsp_vcpu_id = arg;
> +			goto set_boot_cpu_id_out;
> +		}
> +		if (arg > KVM_MAX_VCPU_IDS) {
> +			r = -EINVAL;
> +			goto set_boot_cpu_id_out;
> +		}
> +		kvm->arch.bsp_vcpu_id = arg;
> +set_boot_cpu_id_out:

Any reason not to check kvm->arch.max_vcpu_ids?  It's not super pretty, but it's
also not super hard.

And rather than use gotos, this can be done with if-elif-else, e.g. with the
below delta get to:

		r = 0;
		mutex_lock(&kvm->lock);
		if (kvm->created_vcpus)
			r = -EBUSY;
		else if (arg > KVM_MAX_VCPU_IDS ||
			 (kvm->arch.max_vcpu_ids && arg > kvm->arch.max_vcpu_ids))
			r = -EINVAL;
		else
			kvm->arch.bsp_vcpu_id = arg;
		mutex_unlock(&kvm->lock);
		break;


diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6e6f3d31cff0..994aa281b07d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6707,7 +6707,9 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
                        break;
 
                mutex_lock(&kvm->lock);
-               if (kvm->arch.max_vcpu_ids == cap->args[0]) {
+               if (kvm->arch.bsp_vcpu_id > cap->args[0]) {
+                       ;
+               } else if (kvm->arch.max_vcpu_ids == cap->args[0]) {
                        r = 0;
                } else if (!kvm->arch.max_vcpu_ids) {
                        kvm->arch.max_vcpu_ids = cap->args[0];
@@ -7226,16 +7228,13 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
        case KVM_SET_BOOT_CPU_ID:
                r = 0;
                mutex_lock(&kvm->lock);
-               if (kvm->created_vcpus) {
+               if (kvm->created_vcpus)
                        r = -EBUSY;
-                       goto set_boot_cpu_id_out;
-               }
-               if (arg > KVM_MAX_VCPU_IDS) {
+               else if (arg > KVM_MAX_VCPU_IDS ||
+                        (kvm->arch.max_vcpu_ids && arg > kvm->arch.max_vcpu_ids))
                        r = -EINVAL;
-                       goto set_boot_cpu_id_out;
-               }
-               kvm->arch.bsp_vcpu_id = arg;
-set_boot_cpu_id_out:
+               else
+                       kvm->arch.bsp_vcpu_id = arg;
                mutex_unlock(&kvm->lock);
                break;
 #ifdef CONFIG_KVM_XEN

Re: [PATCH v2 3/4] KVM: Limit check IDs for KVM_SET_BOOT_CPU_ID

[Mathias Krause]

On 14.06.24 18:35, Sean Christopherson wrote:
> On Wed, Jun 12, 2024, Mathias Krause wrote:
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 082ac6d95a3a..8bc7b8b2dfc5 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -7220,10 +7220,16 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>>  	case KVM_SET_BOOT_CPU_ID:
>>  		r = 0;
>>  		mutex_lock(&kvm->lock);
>> -		if (kvm->created_vcpus)
>> +		if (kvm->created_vcpus) {
>>  			r = -EBUSY;
>> -		else
>> -			kvm->arch.bsp_vcpu_id = arg;
>> +			goto set_boot_cpu_id_out;
>> +		}
>> +		if (arg > KVM_MAX_VCPU_IDS) {
>> +			r = -EINVAL;
>> +			goto set_boot_cpu_id_out;
>> +		}
>> +		kvm->arch.bsp_vcpu_id = arg;
>> +set_boot_cpu_id_out:
> 
> Any reason not to check kvm->arch.max_vcpu_ids?  It's not super pretty, but it's
> also not super hard.

I explicitly excluded it as there's no strict requirement for calling
KVM_ENABLE_CAP(KVM_CAP_MAX_VCPU_ID) before KVM_SET_BOOT_CPU_ID, so
kvm->arch.max_vcpu_ids would not yet be set. But yeah, checking if it
was already set prior to narrowing the compare to it is a neat way to
solve that. Good idea!

> And rather than use gotos, this can be done with if-elif-else, e.g. with the
> below delta get to:
> 
> 		r = 0;
> 		mutex_lock(&kvm->lock);
> 		if (kvm->created_vcpus)
> 			r = -EBUSY;
> 		else if (arg > KVM_MAX_VCPU_IDS ||
> 			 (kvm->arch.max_vcpu_ids && arg > kvm->arch.max_vcpu_ids))
> 			r = -EINVAL;
> 		else
> 			kvm->arch.bsp_vcpu_id = arg;
> 		mutex_unlock(&kvm->lock);
> 		break;

Heh, I had the if-else ladder before but went for the goto version after
looking around, attempting not to deviate from the "style" used for all
the other cases . :|

> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6e6f3d31cff0..994aa281b07d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6707,7 +6707,9 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>                         break;
>  
>                 mutex_lock(&kvm->lock);
> -               if (kvm->arch.max_vcpu_ids == cap->args[0]) {
> +               if (kvm->arch.bsp_vcpu_id > cap->args[0]) {
> +                       ;
> +               } else if (kvm->arch.max_vcpu_ids == cap->args[0]) {
>                         r = 0;
>                 } else if (!kvm->arch.max_vcpu_ids) {
>                         kvm->arch.max_vcpu_ids = cap->args[0];

This is a separate fix, imho -- not allowing to set
kvm->arch.max_vcpu_ids to a value that would exclude
kvm->arch.bsp_vcpu_id. So I'll put that a separate commit.

However, this still doesn't prevent creating VMs that have no BSP as the
actual vCPU ID assignment only happens later, when vCPUs are created.

But, I guess, that's no real issue. If userland insists on not having a
BSP, so be it.

> @@ -7226,16 +7228,13 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
>         case KVM_SET_BOOT_CPU_ID:
>                 r = 0;
>                 mutex_lock(&kvm->lock);
> -               if (kvm->created_vcpus) {
> +               if (kvm->created_vcpus)
>                         r = -EBUSY;
> -                       goto set_boot_cpu_id_out;
> -               }
> -               if (arg > KVM_MAX_VCPU_IDS) {
> +               else if (arg > KVM_MAX_VCPU_IDS ||
> +                        (kvm->arch.max_vcpu_ids && arg > kvm->arch.max_vcpu_ids))
>                         r = -EINVAL;
> -                       goto set_boot_cpu_id_out;
> -               }
> -               kvm->arch.bsp_vcpu_id = arg;
> -set_boot_cpu_id_out:
> +               else
> +                       kvm->arch.bsp_vcpu_id = arg;
>                 mutex_unlock(&kvm->lock);
>                 break;
>  #ifdef CONFIG_KVM_XEN

Thanks, looking good!

Will prepare a v3.

Mathias

Re: [PATCH v2 3/4] KVM: Limit check IDs for KVM_SET_BOOT_CPU_ID

[Sean Christopherson]

On Fri, Jun 14, 2024, Mathias Krause wrote:
> On 14.06.24 18:35, Sean Christopherson wrote:
> However, this still doesn't prevent creating VMs that have no BSP as the
> actual vCPU ID assignment only happens later, when vCPUs are created.
> 
> But, I guess, that's no real issue. If userland insists on not having a
> BSP, so be it.

"struct kvm" is zero-allocated, so the BSP will default to vCPU0.  I wouldn't be
at all surprised if VMMs rely on that (after looking, that does appear to be the
case for our VMM).

Re: [PATCH v2 3/4] KVM: Limit check IDs for KVM_SET_BOOT_CPU_ID

[Mathias Krause]

On 14.06.24 22:36, Sean Christopherson wrote:
> On Fri, Jun 14, 2024, Mathias Krause wrote:
>> On 14.06.24 18:35, Sean Christopherson wrote:
>> However, this still doesn't prevent creating VMs that have no BSP as the
>> actual vCPU ID assignment only happens later, when vCPUs are created.
>>
>> But, I guess, that's no real issue. If userland insists on not having a
>> BSP, so be it.
> 
> "struct kvm" is zero-allocated, so the BSP will default to vCPU0.  I wouldn't be
> at all surprised if VMMs rely on that (after looking, that does appear to be the
> case for our VMM).

Sure, zero-initialized by default makes a lot of sense. I too would
assume that CPU0 is the BSP.

However, I was thinking more along the lines:

--- a/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
+++ b/tools/testing/selftests/kvm/x86_64/set_boot_cpu_id.c
@@ -136,6 +136,7 @@ int main(int argc, char *argv[])
        run_vm_bsp(0);
        run_vm_bsp(1);
        run_vm_bsp(0);
+       run_vm_bsp(42);

        check_set_bsp_busy();
 }

As in: having two vCPUs with IDs 0 and 1 but the BSP with an ID outside
of that range, e.g. 42 in this case.

That's still a working setup but without any dedicated BSP, so may cause
some hickups for real operating systems. But, again, if a user does this
on purpose and things break, well, can keep the pieces.

Thanks,
Mathias