kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/5] KVM: Reject vCPU IDs above 2^32
@ 2024-06-14 20:28 Mathias Krause
  2024-06-14 20:28 ` [PATCH v3 1/5] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU Mathias Krause
                   ` (5 more replies)
  0 siblings, 6 replies; 11+ messages in thread
From: Mathias Krause @ 2024-06-14 20:28 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson; +Cc: kvm, Mathias Krause

This small series evolved from a single vCPU ID limit check to
multiple ones, including sanity checks among them and enhanced
selftests.

Please apply!

Thanks,
Mathias

v2: https://lore.kernel.org/kvm/20240612215415.3450952-1-minipli@grsecurity.net/

changes v2->v3:
- test max_vcpu_ids on bsp_vcpu_id limit check if already set (Sean)
- add patch to check max_vcpu_ids to not exclude bsp_vcpu_id (Sean)
- reorder series to have selftests at the end
- minor commit log cosmetics

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: x86: Limit check IDs for KVM_SET_BOOT_CPU_ID
  KVM: selftests: Test max vCPU IDs corner cases
  KVM: selftests: Test vCPU boot IDs above 2^32

Sean Christopherson (1):
  KVM: x86: Prevent excluding the BSP on setting max_vcpu_ids

 arch/x86/kvm/x86.c                            |  7 +++++-
 .../kvm/x86_64/max_vcpuid_cap_test.c          | 22 +++++++++++++++++--
 .../selftests/kvm/x86_64/set_boot_cpu_id.c    | 11 ++++++++++
 virt/kvm/kvm_main.c                           | 10 ++++++++-
 4 files changed, 46 insertions(+), 4 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3 1/5] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU
  2024-06-14 20:28 [PATCH v3 0/5] KVM: Reject vCPU IDs above 2^32 Mathias Krause
@ 2024-06-14 20:28 ` Mathias Krause
  2024-06-18 21:43   ` Sean Christopherson
  2024-06-14 20:28 ` [PATCH v3 2/5] KVM: x86: Limit check IDs for KVM_SET_BOOT_CPU_ID Mathias Krause
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 11+ messages in thread
From: Mathias Krause @ 2024-06-14 20:28 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson
  Cc: kvm, Mathias Krause, Emese Revfy, PaX Team

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>
---
 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 2/5] KVM: x86: Limit check IDs for KVM_SET_BOOT_CPU_ID
  2024-06-14 20:28 [PATCH v3 0/5] KVM: Reject vCPU IDs above 2^32 Mathias Krause
  2024-06-14 20:28 ` [PATCH v3 1/5] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU Mathias Krause
@ 2024-06-14 20:28 ` Mathias Krause
  2024-06-14 20:28 ` [PATCH v3 3/5] KVM: x86: Prevent excluding the BSP on setting max_vcpu_ids Mathias Krause
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Mathias Krause @ 2024-06-14 20:28 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson; +Cc: kvm, Mathias Krause

Do not accept IDs which are definitely invalid by limit checking the
passed value against KVM_MAX_VCPU_IDS and 'max_vcpu_ids' if it was
already set.

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.")
Co-developed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
v3:
- check against arch.max_vcpu_ids too, if already set (Sean)
- switch to else-if instead of goto (Sean)

 arch/x86/kvm/x86.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 082ac6d95a3a..686606f61dee 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7222,6 +7222,9 @@ int kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)
 		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);
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 3/5] KVM: x86: Prevent excluding the BSP on setting max_vcpu_ids
  2024-06-14 20:28 [PATCH v3 0/5] KVM: Reject vCPU IDs above 2^32 Mathias Krause
  2024-06-14 20:28 ` [PATCH v3 1/5] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU Mathias Krause
  2024-06-14 20:28 ` [PATCH v3 2/5] KVM: x86: Limit check IDs for KVM_SET_BOOT_CPU_ID Mathias Krause
@ 2024-06-14 20:28 ` Mathias Krause
  2024-06-14 20:28 ` [PATCH v3 4/5] KVM: selftests: Test max vCPU IDs corner cases Mathias Krause
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 11+ messages in thread
From: Mathias Krause @ 2024-06-14 20:28 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson; +Cc: kvm, Mathias Krause

From: Sean Christopherson <seanjc@google.com>

If the BSP vCPU ID was already set, ensure it doesn't get excluded when
limiting vCPU IDs via KVM_CAP_MAX_VCPU_ID.

[mks: provide commit message, code by Sean]
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 arch/x86/kvm/x86.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 686606f61dee..eeb35fb1573a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6701,7 +6701,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];
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 4/5] KVM: selftests: Test max vCPU IDs corner cases
  2024-06-14 20:28 [PATCH v3 0/5] KVM: Reject vCPU IDs above 2^32 Mathias Krause
                   ` (2 preceding siblings ...)
  2024-06-14 20:28 ` [PATCH v3 3/5] KVM: x86: Prevent excluding the BSP on setting max_vcpu_ids Mathias Krause
@ 2024-06-14 20:28 ` Mathias Krause
  2024-06-18 21:46   ` Sean Christopherson
  2024-06-14 20:28 ` [PATCH v3 5/5] KVM: selftests: Test vCPU boot IDs above 2^32 Mathias Krause
  2024-06-18 21:41 ` [PATCH v3 0/5] KVM: Reject vCPU " Sean Christopherson
  5 siblings, 1 reply; 11+ messages in thread
From: Mathias Krause @ 2024-06-14 20:28 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson; +Cc: kvm, 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.

It also allowed excluding a once set boot CPU ID.

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

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
v3:
- test BOOT_CPU_ID interaction too

 .../kvm/x86_64/max_vcpuid_cap_test.c          | 22 +++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

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..c2da915201be 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
@@ -26,19 +26,37 @@ int main(int argc, char *argv[])
 	TEST_ASSERT(ret < 0,
 		    "Setting KVM_CAP_MAX_VCPU_ID beyond KVM cap should fail");
 
+	/* Test BOOT_CPU_ID interaction (MAX_VCPU_ID cannot be lower) */
+	if (kvm_has_cap(KVM_CAP_SET_BOOT_CPU_ID)) {
+		vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *)MAX_VCPU_ID);
+
+		/* Try setting KVM_CAP_MAX_VCPU_ID below BOOT_CPU_ID */
+		ret = __vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID - 1);
+		TEST_ASSERT(ret < 0,
+			    "Setting KVM_CAP_MAX_VCPU_ID below BOOT_CPU_ID should fail");
+	}
+
 	/* Set KVM_CAP_MAX_VCPU_ID */
 	vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID);
 
-
 	/* Try to set KVM_CAP_MAX_VCPU_ID again */
 	ret = __vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID + 1);
 	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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v3 5/5] KVM: selftests: Test vCPU boot IDs above 2^32
  2024-06-14 20:28 [PATCH v3 0/5] KVM: Reject vCPU IDs above 2^32 Mathias Krause
                   ` (3 preceding siblings ...)
  2024-06-14 20:28 ` [PATCH v3 4/5] KVM: selftests: Test max vCPU IDs corner cases Mathias Krause
@ 2024-06-14 20:28 ` Mathias Krause
  2024-06-18 21:50   ` Sean Christopherson
  2024-06-18 21:41 ` [PATCH v3 0/5] KVM: Reject vCPU " Sean Christopherson
  5 siblings, 1 reply; 11+ messages in thread
From: Mathias Krause @ 2024-06-14 20:28 UTC (permalink / raw)
  To: Paolo Bonzini, Sean Christopherson; +Cc: kvm, 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 0/5] KVM: Reject vCPU IDs above 2^32
  2024-06-14 20:28 [PATCH v3 0/5] KVM: Reject vCPU IDs above 2^32 Mathias Krause
                   ` (4 preceding siblings ...)
  2024-06-14 20:28 ` [PATCH v3 5/5] KVM: selftests: Test vCPU boot IDs above 2^32 Mathias Krause
@ 2024-06-18 21:41 ` Sean Christopherson
  2024-06-19  6:28   ` Mathias Krause
  5 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2024-06-18 21:41 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini, Mathias Krause; +Cc: kvm

On Fri, 14 Jun 2024 22:28:54 +0200, Mathias Krause wrote:
> This small series evolved from a single vCPU ID limit check to
> multiple ones, including sanity checks among them and enhanced
> selftests.

Applied to kvm-x86 generic, with a few tweaks (emails incoming).  Thanks!

[1/5] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU
      https://github.com/kvm-x86/linux/commit/8b8e57e5096e
[2/5] KVM: x86: Limit check IDs for KVM_SET_BOOT_CPU_ID
      https://github.com/kvm-x86/linux/commit/7c305d5118e6
[3/5] KVM: x86: Prevent excluding the BSP on setting max_vcpu_ids
      https://github.com/kvm-x86/linux/commit/d29bf2ca1404
[4/5] KVM: selftests: Test max vCPU IDs corner cases
      https://github.com/kvm-x86/linux/commit/4b451a57809c
[5/5] KVM: selftests: Test vCPU boot IDs above 2^32
      https://github.com/kvm-x86/linux/commit/438a496b9041

--
https://github.com/kvm-x86/linux/tree/next

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 1/5] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU
  2024-06-14 20:28 ` [PATCH v3 1/5] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU Mathias Krause
@ 2024-06-18 21:43   ` Sean Christopherson
  0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2024-06-18 21:43 UTC (permalink / raw)
  To: Mathias Krause; +Cc: Paolo Bonzini, kvm, Emese Revfy, PaX Team

On Fri, Jun 14, 2024, Mathias Krause wrote:
> 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.

I tweaked this slightly because it's not just accidental changes we need to
guard against, and to "hint" that vcpu_id really should be an "unsigned int".

	/*
	 * KVM tracks vCPU IDs as 'int', be kind to userspace and reject
	 * too-large values instead of silently truncating.
	 *
	 * Ensure KVM_MAX_VCPU_IDS isn't pushed above INT_MAX without first
	 * changing the storage type (at the very least, IDs should be tracked
	 * as unsigned ints).
	 */

> +	 */
> +	BUILD_BUG_ON(KVM_MAX_VCPU_IDS > INT_MAX);
>  	if (id >= KVM_MAX_VCPU_IDS)
>  		return -EINVAL;
>  
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 4/5] KVM: selftests: Test max vCPU IDs corner cases
  2024-06-14 20:28 ` [PATCH v3 4/5] KVM: selftests: Test max vCPU IDs corner cases Mathias Krause
@ 2024-06-18 21:46   ` Sean Christopherson
  0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2024-06-18 21:46 UTC (permalink / raw)
  To: Mathias Krause; +Cc: Paolo Bonzini, kvm

On Fri, Jun 14, 2024, Mathias Krause wrote:
> 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.
> 
> It also allowed excluding a once set boot CPU ID.
> 
> Verify this no longer works and gets rejected with an error.
> 
> Signed-off-by: Mathias Krause <minipli@grsecurity.net>
> ---
> v3:
> - test BOOT_CPU_ID interaction too
> 
>  .../kvm/x86_64/max_vcpuid_cap_test.c          | 22 +++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> 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..c2da915201be 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
> @@ -26,19 +26,37 @@ int main(int argc, char *argv[])
>  	TEST_ASSERT(ret < 0,
>  		    "Setting KVM_CAP_MAX_VCPU_ID beyond KVM cap should fail");
>  
> +	/* Test BOOT_CPU_ID interaction (MAX_VCPU_ID cannot be lower) */
> +	if (kvm_has_cap(KVM_CAP_SET_BOOT_CPU_ID)) {
> +		vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *)MAX_VCPU_ID);
> +
> +		/* Try setting KVM_CAP_MAX_VCPU_ID below BOOT_CPU_ID */
> +		ret = __vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID - 1);
> +		TEST_ASSERT(ret < 0,
> +			    "Setting KVM_CAP_MAX_VCPU_ID below BOOT_CPU_ID should fail");
> +	}
> +
>  	/* Set KVM_CAP_MAX_VCPU_ID */
>  	vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID);
>  
> -
>  	/* Try to set KVM_CAP_MAX_VCPU_ID again */
>  	ret = __vm_enable_cap(vm, KVM_CAP_MAX_VCPU_ID, MAX_VCPU_ID + 1);
>  	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 */

I changed this comment to

	/* Create vCPU with bits 63:32 != 0, but an otherwise valid id */

mostly because it's specifically testing the bad truncation of the upper bits,
but also because I initially misinterpreted the intent and confused it with the
INT_MAX BUILD_BUG_ON().

> +	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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 5/5] KVM: selftests: Test vCPU boot IDs above 2^32
  2024-06-14 20:28 ` [PATCH v3 5/5] KVM: selftests: Test vCPU boot IDs above 2^32 Mathias Krause
@ 2024-06-18 21:50   ` Sean Christopherson
  0 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2024-06-18 21:50 UTC (permalink / raw)
  To: Mathias Krause; +Cc: Paolo Bonzini, kvm

On Fri, Jun 14, 2024, Mathias Krause wrote:
> 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));
> +

I also added a test to verify KVM_CAP_MAX_VCPU_ID+1 also fails, because why not.

	unsigned long max_vcpu_id = vm_check_cap(vm, KVM_CAP_MAX_VCPU_ID);
	int r;

	if (max_vcpu_id) {
		r = __vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *)(max_vcpu_id + 1));
		TEST_ASSERT(r == -1 && errno == EINVAL, "BSP with ID > MAX should fail");
	}

	r = __vm_ioctl(vm, KVM_SET_BOOT_CPU_ID, (void *)(1L << 32));
	TEST_ASSERT(r == -1 && errno == EINVAL, "BSP with ID[63:32]!=0 should fail");

> +	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++)

I dropped this and just had every VM run the negative test.  There's zero chance
anyone will ever notice an extra failed ioctl() or three, whereas it took me a
second to realize this is just a somewhat lazy way of writing a one-off negative
test.

> +		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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3 0/5] KVM: Reject vCPU IDs above 2^32
  2024-06-18 21:41 ` [PATCH v3 0/5] KVM: Reject vCPU " Sean Christopherson
@ 2024-06-19  6:28   ` Mathias Krause
  0 siblings, 0 replies; 11+ messages in thread
From: Mathias Krause @ 2024-06-19  6:28 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, Paolo Bonzini

On 18.06.24 23:41, Sean Christopherson wrote:
> On Fri, 14 Jun 2024 22:28:54 +0200, Mathias Krause wrote:
>> This small series evolved from a single vCPU ID limit check to
>> multiple ones, including sanity checks among them and enhanced
>> selftests.
> 
> Applied to kvm-x86 generic, with a few tweaks (emails incoming).  Thanks!
> 
> [1/5] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU
>       https://github.com/kvm-x86/linux/commit/8b8e57e5096e
> [2/5] KVM: x86: Limit check IDs for KVM_SET_BOOT_CPU_ID
>       https://github.com/kvm-x86/linux/commit/7c305d5118e6
> [3/5] KVM: x86: Prevent excluding the BSP on setting max_vcpu_ids
>       https://github.com/kvm-x86/linux/commit/d29bf2ca1404
> [4/5] KVM: selftests: Test max vCPU IDs corner cases
>       https://github.com/kvm-x86/linux/commit/4b451a57809c
> [5/5] KVM: selftests: Test vCPU boot IDs above 2^32
>       https://github.com/kvm-x86/linux/commit/438a496b9041

Looking good, thanks Sean!

Mathias

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-06-19  6:28 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-14 20:28 [PATCH v3 0/5] KVM: Reject vCPU IDs above 2^32 Mathias Krause
2024-06-14 20:28 ` [PATCH v3 1/5] KVM: Reject overly excessive IDs in KVM_CREATE_VCPU Mathias Krause
2024-06-18 21:43   ` Sean Christopherson
2024-06-14 20:28 ` [PATCH v3 2/5] KVM: x86: Limit check IDs for KVM_SET_BOOT_CPU_ID Mathias Krause
2024-06-14 20:28 ` [PATCH v3 3/5] KVM: x86: Prevent excluding the BSP on setting max_vcpu_ids Mathias Krause
2024-06-14 20:28 ` [PATCH v3 4/5] KVM: selftests: Test max vCPU IDs corner cases Mathias Krause
2024-06-18 21:46   ` Sean Christopherson
2024-06-14 20:28 ` [PATCH v3 5/5] KVM: selftests: Test vCPU boot IDs above 2^32 Mathias Krause
2024-06-18 21:50   ` Sean Christopherson
2024-06-18 21:41 ` [PATCH v3 0/5] KVM: Reject vCPU " Sean Christopherson
2024-06-19  6:28   ` Mathias Krause

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).