On Fri, Sep 6, 2019 at 7:27 PM Sean Christopherson
<sean.j.christopherson@xxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> > 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.
