KVM Archive on lore.kernel.org
 help / color / Atom feed
* [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs
@ 2019-09-06 16:34 Sean Christopherson
  2019-09-06 16:34 ` [kvm-unit-tests PATCH 1/3] x86: Fix out of bounds access when processing online_cpus Sean Christopherson
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Sean Christopherson @ 2019-09-06 16:34 UTC (permalink / raw)
  To: Paolo Bonzini, Radim Krčmář; +Cc: kvm, Evgeny Yakovlev

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


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

* [kvm-unit-tests PATCH 1/3] x86: Fix out of bounds access when processing online_cpus
  2019-09-06 16:34 [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs Sean Christopherson
@ 2019-09-06 16:34 ` Sean Christopherson
  2019-09-06 16:34 ` [kvm-unit-tests PATCH 2/3] x86: Declare online_cpus based on MAX_TEST_CPUS Sean Christopherson
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2019-09-06 16:34 UTC (permalink / raw)
  To: Paolo Bonzini, Radim Krčmář; +Cc: kvm, Evgeny Yakovlev

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


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

* [kvm-unit-tests PATCH 2/3] x86: Declare online_cpus based on MAX_TEST_CPUS
  2019-09-06 16:34 [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs Sean Christopherson
  2019-09-06 16:34 ` [kvm-unit-tests PATCH 1/3] x86: Fix out of bounds access when processing online_cpus Sean Christopherson
@ 2019-09-06 16:34 ` Sean Christopherson
  2019-09-06 16:34 ` [kvm-unit-tests PATCH 3/3] x86: Bump max number of test CPUs to 255 Sean Christopherson
  2019-09-10  7:57 ` [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs Peter Xu
  3 siblings, 0 replies; 6+ messages in thread
From: Sean Christopherson @ 2019-09-06 16:34 UTC (permalink / raw)
  To: Paolo Bonzini, Radim Krčmář; +Cc: kvm, Evgeny Yakovlev

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


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

* [kvm-unit-tests PATCH 3/3] x86: Bump max number of test CPUs to 255
  2019-09-06 16:34 [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs Sean Christopherson
  2019-09-06 16:34 ` [kvm-unit-tests PATCH 1/3] x86: Fix out of bounds access when processing online_cpus Sean Christopherson
  2019-09-06 16:34 ` [kvm-unit-tests PATCH 2/3] x86: Declare online_cpus based on MAX_TEST_CPUS Sean Christopherson
@ 2019-09-06 16:34 ` Sean Christopherson
  2019-09-10 14:46   ` Paolo Bonzini
  2019-09-10  7:57 ` [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs Peter Xu
  3 siblings, 1 reply; 6+ messages in thread
From: Sean Christopherson @ 2019-09-06 16:34 UTC (permalink / raw)
  To: Paolo Bonzini, Radim Krčmář; +Cc: kvm, Evgeny Yakovlev

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


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

* Re: [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs
  2019-09-06 16:34 [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs Sean Christopherson
                   ` (2 preceding siblings ...)
  2019-09-06 16:34 ` [kvm-unit-tests PATCH 3/3] x86: Bump max number of test CPUs to 255 Sean Christopherson
@ 2019-09-10  7:57 ` Peter Xu
  3 siblings, 0 replies; 6+ messages in thread
From: Peter Xu @ 2019-09-10  7:57 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Paolo Bonzini, Radim Krčmář, kvm, Evgeny Yakovlev

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

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

* Re: [kvm-unit-tests PATCH 3/3] x86: Bump max number of test CPUs to 255
  2019-09-06 16:34 ` [kvm-unit-tests PATCH 3/3] x86: Bump max number of test CPUs to 255 Sean Christopherson
@ 2019-09-10 14:46   ` Paolo Bonzini
  0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2019-09-10 14:46 UTC (permalink / raw)
  To: Sean Christopherson, Radim Krčmář; +Cc: kvm, Evgeny Yakovlev

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

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

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-06 16:34 [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs Sean Christopherson
2019-09-06 16:34 ` [kvm-unit-tests PATCH 1/3] x86: Fix out of bounds access when processing online_cpus Sean Christopherson
2019-09-06 16:34 ` [kvm-unit-tests PATCH 2/3] x86: Declare online_cpus based on MAX_TEST_CPUS Sean Christopherson
2019-09-06 16:34 ` [kvm-unit-tests PATCH 3/3] x86: Bump max number of test CPUs to 255 Sean Christopherson
2019-09-10 14:46   ` Paolo Bonzini
2019-09-10  7:57 ` [kvm-unit-tests PATCH 0/3] x86: Cleanup max test CPUs Peter Xu

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org kvm@archiver.kernel.org
	public-inbox-index kvm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox