Re: [Qemu-devel] [PATCH] target/ppc: Fix serious bug in HPTE writeback

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2667 bytes --]

On Tue, Feb 21, 2017 at 01:52:11PM +1100, David Gibson wrote:
> ppc_hash64_store_hpte() is used to update HPTEs in the hashed page table
> (HPT) for 64-bit machines.  This is used when the (emulated) CPU needs to
> update the referenced (R) or changed (C) bits in the HPTE.
> 
> Some time ago this was converted to take an HPTE index, instead of a
> raw offset to the HPTE within the HPT (similar functions for 32-bit still
> take an offset).  In the process a serious bug was introduced: we're
> still using the index parameter as though it was an offset, failing to
> multiply by the size of an HPTE, so it will update bits in the wrong part
> of the HPT.  This can corrupt the guests's HPT, causing crashes or data
> loss.
> 
> AFAICT the reason we haven't noticed this error earlier is that for 64-bit
> machines we've been testing almost exclusively with Linux guests.  Linux
> on ppc does not make use of the hardware R & C bits, so this writeback
> will never be triggered.  It also occurs only on TCG, not KVM, guests.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Self NACK.  Sorry.  In my panic, I managed to miss the multiply a few
lines above.  There isn't actually a bug here, just some confusing
variable naming.

> ---
>  target/ppc/mmu-hash64.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index bb78fb5..dc3b5f7 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -894,12 +894,15 @@ void ppc_hash64_store_hpte(PowerPCCPU *cpu,
>  
>      pte_index *= HASH_PTE_SIZE_64;
>      if (env->external_htab) {
> -        stq_p(env->external_htab + pte_index, pte0);
> -        stq_p(env->external_htab + pte_index + HASH_PTE_SIZE_64 / 2, pte1);
> +        stq_p(env->external_htab + pte_index * HASH_PTE_SIZE_64, pte0);
> +        stq_p(env->external_htab + pte_index * HASH_PTE_SIZE_64
> +              + HASH_PTE_SIZE_64 / 2, pte1);
>      } else {
> -        stq_phys(CPU(cpu)->as, env->htab_base + pte_index, pte0);
>          stq_phys(CPU(cpu)->as,
> -                 env->htab_base + pte_index + HASH_PTE_SIZE_64 / 2, pte1);
> +                 env->htab_base + pte_index * HASH_PTE_SIZE_64, pte0);
> +        stq_phys(CPU(cpu)->as,
> +                 env->htab_base + pte_index * HASH_PTE_SIZE_64
> +                 + HASH_PTE_SIZE_64 / 2, pte1);
>      }
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]