KVM Archive on lore.kernel.org
 help / color / Atom feed
* Re: [kvm-unit-tests RESEND PATCH] x86: Fix id_map buffer overflow and PT corruption
       [not found] <1566979099-23628-1-git-send-email-wrfsh@yandex-team.ru>
@ 2019-09-11 15:37 ` Paolo Bonzini
  0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2019-09-11 15:37 UTC (permalink / raw)
  To: Evgeny Yakovlev, kvm; +Cc: rkrcmar, yc-core

On 28/08/19 09:58, Evgeny Yakovlev wrote:
> Commit 18a34cce introduced init_apic_map. It iterates over
> sizeof(online_cpus) * 8 items and sets APIC ids in id_map.
> However, online_cpus is defined (in x86/cstart[64].S) as a 64-bit
> variable. After i >= 64, init_apic_map begins to read out of bounds of
> online_cpus. If it finds a non-zero value there enough times,
> it then proceeds to potentially overflow id_map in assignment.
> 
> In our test case id_map was linked close to pg_base. As a result page
> table was corrupted and we've seen sporadic failures of ioapic test.
> 
> Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>

Superseded by

[PATCH] x86: Fix out of bounds access when processing online_cpus
[PATCH] x86: Declare online_cpus based on MAX_TEST_CPUS
[PATCH] x86: Bump max number of test CPUs to 255


Thanks,

Paolo

> ---
>  lib/x86/apic.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/x86/apic.c b/lib/x86/apic.c
> index 504299e..1ed8bab 100644
> --- a/lib/x86/apic.c
> +++ b/lib/x86/apic.c
> @@ -228,14 +228,17 @@ void mask_pic_interrupts(void)
>      outb(0xff, 0xa1);
>  }
>  
> -extern unsigned char online_cpus[256 / 8];
> +/* Should hold MAX_TEST_CPUS bits */
> +extern uint64_t online_cpus;
>  
>  void init_apic_map(void)
>  {
>  	unsigned int i, j = 0;
>  
> -	for (i = 0; i < sizeof(online_cpus) * 8; i++) {
> -		if ((1ul << (i % 8)) & (online_cpus[i / 8]))
> +	assert(MAX_TEST_CPUS <= sizeof(online_cpus) * 8);
> +
> +	for (i = 0; i < MAX_TEST_CPUS; i++) {
> +		if (online_cpus & ((uint64_t)1 << i))
>  			id_map[j++] = i;
>  	}
>  }
> 


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

* Re: [kvm-unit-tests RESEND PATCH] x86: Fix id_map buffer overflow and PT corruption
  2019-09-06 16:27 ` Sean Christopherson
@ 2019-09-09  9:15   ` Evgeny Yakovlev
  0 siblings, 0 replies; 4+ messages in thread
From: Evgeny Yakovlev @ 2019-09-09  9:15 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: kvm, pbonzini, rkrcmar, yc-core, wrfsh

On Fri, Sep 6, 2019 at 7:27 PM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Fri, Sep 06, 2019 at 10:49:19AM +0300, Evgeny Yakovlev wrote:
> > Commit 18a34cce introduced init_apic_map. It iterates over
> > sizeof(online_cpus) * 8 items and sets APIC ids in id_map.
> > However, online_cpus is defined (in x86/cstart[64].S) as a 64-bit
> > variable. After i >= 64, init_apic_map begins to read out of bounds of
> > online_cpus. If it finds a non-zero value there enough times,
> > it then proceeds to potentially overflow id_map in assignment.
> >
> > In our test case id_map was linked close to pg_base. As a result page
> > table was corrupted and we've seen sporadic failures of ioapic test.
> >
> > Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>
> > ---
> >  lib/x86/apic.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/x86/apic.c b/lib/x86/apic.c
> > index 504299e..1ed8bab 100644
> > --- a/lib/x86/apic.c
> > +++ b/lib/x86/apic.c
> > @@ -228,14 +228,17 @@ void mask_pic_interrupts(void)
> >      outb(0xff, 0xa1);
> >  }
> >
> > -extern unsigned char online_cpus[256 / 8];
>
> The immediate issue can be resolved simply by fixing this definition.
>
> > +/* Should hold MAX_TEST_CPUS bits */
> > +extern uint64_t online_cpus;
> >
> >  void init_apic_map(void)
> >  {
> >       unsigned int i, j = 0;
> >
> > -     for (i = 0; i < sizeof(online_cpus) * 8; i++) {
> > -             if ((1ul << (i % 8)) & (online_cpus[i / 8]))
> > +     assert(MAX_TEST_CPUS <= sizeof(online_cpus) * 8);
> > +
> > +     for (i = 0; i < MAX_TEST_CPUS; i++) {
> > +             if (online_cpus & ((uint64_t)1 << i))
>
> This is functionally correct, but it's just as easy to have online_cpus
> sized based on MAX_TEST_CPUS, i.e. to allow MAX_TEST_CPUS to be changed
> at will (within reason).  I'll send patches.
>
> >                       id_map[j++] = i;
> >       }
> >  }
> > --
> > 2.7.4
> >

Yeah, you can fix this declaration here as well, using MAX_TEST_CPUS.
I just don't like the definition (which is in x86/start64.S) to be
different from this declaration here. I think it is confusing.
And since actual definition does not use MAX_TEST_CPUS as well, i
think it is also quite fragile.

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

* Re: [kvm-unit-tests RESEND PATCH] x86: Fix id_map buffer overflow and PT corruption
  2019-09-06  7:49 Evgeny Yakovlev
@ 2019-09-06 16:27 ` Sean Christopherson
  2019-09-09  9:15   ` Evgeny Yakovlev
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Christopherson @ 2019-09-06 16:27 UTC (permalink / raw)
  To: Evgeny Yakovlev; +Cc: kvm, pbonzini, rkrcmar, yc-core, wrfsh

On Fri, Sep 06, 2019 at 10:49:19AM +0300, Evgeny Yakovlev wrote:
> Commit 18a34cce introduced init_apic_map. It iterates over
> sizeof(online_cpus) * 8 items and sets APIC ids in id_map.
> However, online_cpus is defined (in x86/cstart[64].S) as a 64-bit
> variable. After i >= 64, init_apic_map begins to read out of bounds of
> online_cpus. If it finds a non-zero value there enough times,
> it then proceeds to potentially overflow id_map in assignment.
> 
> In our test case id_map was linked close to pg_base. As a result page
> table was corrupted and we've seen sporadic failures of ioapic test.
> 
> Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>
> ---
>  lib/x86/apic.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/x86/apic.c b/lib/x86/apic.c
> index 504299e..1ed8bab 100644
> --- a/lib/x86/apic.c
> +++ b/lib/x86/apic.c
> @@ -228,14 +228,17 @@ void mask_pic_interrupts(void)
>      outb(0xff, 0xa1);
>  }
>  
> -extern unsigned char online_cpus[256 / 8];

The immediate issue can be resolved simply by fixing this definition.

> +/* Should hold MAX_TEST_CPUS bits */
> +extern uint64_t online_cpus;
>  
>  void init_apic_map(void)
>  {
>  	unsigned int i, j = 0;
>  
> -	for (i = 0; i < sizeof(online_cpus) * 8; i++) {
> -		if ((1ul << (i % 8)) & (online_cpus[i / 8]))
> +	assert(MAX_TEST_CPUS <= sizeof(online_cpus) * 8);
> +
> +	for (i = 0; i < MAX_TEST_CPUS; i++) {
> +		if (online_cpus & ((uint64_t)1 << i))

This is functionally correct, but it's just as easy to have online_cpus
sized based on MAX_TEST_CPUS, i.e. to allow MAX_TEST_CPUS to be changed
at will (within reason).  I'll send patches.

>  			id_map[j++] = i;
>  	}
>  }
> -- 
> 2.7.4
> 

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

* [kvm-unit-tests RESEND PATCH] x86: Fix id_map buffer overflow and PT corruption
@ 2019-09-06  7:49 Evgeny Yakovlev
  2019-09-06 16:27 ` Sean Christopherson
  0 siblings, 1 reply; 4+ messages in thread
From: Evgeny Yakovlev @ 2019-09-06  7:49 UTC (permalink / raw)
  To: kvm; +Cc: pbonzini, rkrcmar, yc-core, wrfsh

Commit 18a34cce introduced init_apic_map. It iterates over
sizeof(online_cpus) * 8 items and sets APIC ids in id_map.
However, online_cpus is defined (in x86/cstart[64].S) as a 64-bit
variable. After i >= 64, init_apic_map begins to read out of bounds of
online_cpus. If it finds a non-zero value there enough times,
it then proceeds to potentially overflow id_map in assignment.

In our test case id_map was linked close to pg_base. As a result page
table was corrupted and we've seen sporadic failures of ioapic test.

Signed-off-by: Evgeny Yakovlev <wrfsh@yandex-team.ru>
---
 lib/x86/apic.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/x86/apic.c b/lib/x86/apic.c
index 504299e..1ed8bab 100644
--- a/lib/x86/apic.c
+++ b/lib/x86/apic.c
@@ -228,14 +228,17 @@ void mask_pic_interrupts(void)
     outb(0xff, 0xa1);
 }
 
-extern unsigned char online_cpus[256 / 8];
+/* Should hold MAX_TEST_CPUS bits */
+extern uint64_t online_cpus;
 
 void init_apic_map(void)
 {
 	unsigned int i, j = 0;
 
-	for (i = 0; i < sizeof(online_cpus) * 8; i++) {
-		if ((1ul << (i % 8)) & (online_cpus[i / 8]))
+	assert(MAX_TEST_CPUS <= sizeof(online_cpus) * 8);
+
+	for (i = 0; i < MAX_TEST_CPUS; i++) {
+		if (online_cpus & ((uint64_t)1 << i))
 			id_map[j++] = i;
 	}
 }
-- 
2.7.4


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

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <1566979099-23628-1-git-send-email-wrfsh@yandex-team.ru>
2019-09-11 15:37 ` [kvm-unit-tests RESEND PATCH] x86: Fix id_map buffer overflow and PT corruption Paolo Bonzini
2019-09-06  7:49 Evgeny Yakovlev
2019-09-06 16:27 ` Sean Christopherson
2019-09-09  9:15   ` Evgeny Yakovlev

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