Re: [kvm-unit-tests RESEND PATCH] x86: Fix id_map buffer overflow and PT corruption

[Sean Christopherson <sean.j.christopherson@intel.com>]

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
>