[kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs

[kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs

[Sean Christopherson]

Fix a bug Evgeny reported where init_apic_map() can cause random
corruption due accessing random bytes far beyond the bounds of
online_cpus.  Take the opportunity to bump the max number of test CPUs
to a realistic maximum, i.e. what kvm-unit-tests can support without a
major rework.

Sean Christopherson (3):
  x86: Fix out of bounds access when processing online_cpus
  x86: Declare online_cpus based on MAX_TEST_CPUS
  x86: Bump max number of test CPUs to 255

 lib/x86/apic-defs.h | 2 +-
 lib/x86/apic.c      | 4 ++--
 lib/x86/smp.c       | 2 --
 x86/cstart.S        | 2 +-
 x86/cstart64.S      | 2 +-
 5 files changed, 5 insertions(+), 7 deletions(-)

-- 
2.22.0

[kvm-unit-tests PATCH 1/3] x86: Fix out of bounds access when processing online_cpus

[Sean Christopherson]

online_cpus is misdeclared as a 64 *byte* variable instead of a 64 *bit*
variable.  This causes init_apic_map() to test random bytes when it
iterates over online_cpus, which in turn can cause it to overflow id_map
and corrupt rnadom memory, e.g. pg_base.  Declare online_cpus using
MAX_TEST_CPUS, which presumably is set explicitly to match the storage
size of online_cpus (64-bit values == max of 64 CPUS).

Reported-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 lib/x86/apic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/x86/apic.c b/lib/x86/apic.c
index b3e39ae..f43e9ef 100644
--- a/lib/x86/apic.c
+++ b/lib/x86/apic.c
@@ -232,13 +232,13 @@ void mask_pic_interrupts(void)
     outb(0xff, 0xa1);
 }
 
-extern unsigned char online_cpus[256 / 8];
+extern unsigned char online_cpus[MAX_TEST_CPUS / 8];
 
 void init_apic_map(void)
 {
 	unsigned int i, j = 0;
 
-	for (i = 0; i < sizeof(online_cpus) * 8; i++) {
+	for (i = 0; i < MAX_TEST_CPUS; i++) {
 		if ((1ul << (i % 8)) & (online_cpus[i / 8]))
 			id_map[j++] = i;
 	}
-- 
2.22.0

[kvm-unit-tests PATCH 2/3] x86: Declare online_cpus based on MAX_TEST_CPUS

[Sean Christopherson]

Declare online_cpus so that it is properly sized to have MAX_TEST_CPUS
bits.  Currently, online_cpus is hardcoded to a u64, i.e. changing
MAX_TEST_CPUS to be greater than 64 will result in a variety of out of
bounds accesses.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 lib/x86/smp.c  | 2 --
 x86/cstart.S   | 2 +-
 x86/cstart64.S | 2 +-
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/x86/smp.c b/lib/x86/smp.c
index 30bd1a0..7b1e0e1 100644
--- a/lib/x86/smp.c
+++ b/lib/x86/smp.c
@@ -113,8 +113,6 @@ int cpus_active(void)
     return atomic_read(&active_cpus);
 }
 
-extern unsigned long long online_cpus;
-
 void smp_init(void)
 {
     int i;
diff --git a/x86/cstart.S b/x86/cstart.S
index 575101b..76f6ba5 100644
--- a/x86/cstart.S
+++ b/x86/cstart.S
@@ -208,7 +208,7 @@ smp_init_done:
 	ret
 
 online_cpus:
-	.quad 0
+	.fill max_cpus / 8, 1, 0
 
 cpu_online_count:	.word 1
 
diff --git a/x86/cstart64.S b/x86/cstart64.S
index d4e4652..89ad9f4 100644
--- a/x86/cstart64.S
+++ b/x86/cstart64.S
@@ -282,7 +282,7 @@ idt_descr:
 	.quad boot_idt
 
 online_cpus:
-	.quad 0
+	.fill max_cpus / 8, 1, 0
 
 load_tss:
 	lidtq idt_descr
-- 
2.22.0

[kvm-unit-tests PATCH 3/3] x86: Bump max number of test CPUs to 255

[Sean Christopherson]

The max number of CPUs is not actually enforced anywhere, e.g. manually
setting '-smp 240' when running a unit test will cause random corruption
and hangs during smp initialization.  Increase the max number of test
CPUs to 255, which is the true max kvm-unit-tests can support without
significant changes, e.g. it would need to boot with x2APIC enabled,
support interrupt remapping, etc...

There is no known use case for running with more than 64 CPUs, but the
cost of supporting 255 is minimal, e.g. increases the size of each test
binary by a few kbs and burns a few extra cycles in init_apic_map().

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 lib/x86/apic-defs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/x86/apic-defs.h b/lib/x86/apic-defs.h
index 7107f0f..b2014de 100644
--- a/lib/x86/apic-defs.h
+++ b/lib/x86/apic-defs.h
@@ -6,7 +6,7 @@
  * both in C and ASM
  */
 
-#define MAX_TEST_CPUS (64)
+#define MAX_TEST_CPUS (255)
 
 /*
  * Constants for various Intel APICs. (local APIC, IOAPIC, etc.)
-- 
2.22.0

Re: [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs

[Peter Xu]

On Fri, Sep 06, 2019 at 09:34:47AM -0700, Sean Christopherson wrote:
> Fix a bug Evgeny reported where init_apic_map() can cause random
> corruption due accessing random bytes far beyond the bounds of
> online_cpus.  Take the opportunity to bump the max number of test CPUs
> to a realistic maximum, i.e. what kvm-unit-tests can support without a
> major rework.
> 
> Sean Christopherson (3):
>   x86: Fix out of bounds access when processing online_cpus
>   x86: Declare online_cpus based on MAX_TEST_CPUS
>   x86: Bump max number of test CPUs to 255

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [kvm-unit-tests PATCH 3/3] x86: Bump max number of test CPUs to 255

[Paolo Bonzini]

On 06/09/19 18:34, Sean Christopherson wrote:
> The max number of CPUs is not actually enforced anywhere, e.g. manually
> setting '-smp 240' when running a unit test will cause random corruption
> and hangs during smp initialization.  Increase the max number of test
> CPUs to 255, which is the true max kvm-unit-tests can support without
> significant changes, e.g. it would need to boot with x2APIC enabled,
> support interrupt remapping, etc...
> 
> There is no known use case for running with more than 64 CPUs, but the
> cost of supporting 255 is minimal, e.g. increases the size of each test
> binary by a few kbs and burns a few extra cycles in init_apic_map().
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  lib/x86/apic-defs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/x86/apic-defs.h b/lib/x86/apic-defs.h
> index 7107f0f..b2014de 100644
> --- a/lib/x86/apic-defs.h
> +++ b/lib/x86/apic-defs.h
> @@ -6,7 +6,7 @@
>   * both in C and ASM
>   */
>  
> -#define MAX_TEST_CPUS (64)
> +#define MAX_TEST_CPUS (255)
>  
>  /*
>   * Constants for various Intel APICs. (local APIC, IOAPIC, etc.)
> 

Since this is not a multiple of 8 anymore, the previous patch should
have used (max_cpus + 7) / 8.  Fixed that and queued.

Paolo