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

David Gibson posted 1 patch 142 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170221025211.30007-1-david@gibson.dropbear.id.au
Test checkpatch passed
Test docker passed
Test s390x passed
target/ppc/mmu-hash64.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)

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

Posted by David Gibson 142 weeks ago
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>
---
 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);
     }
 }
 
-- 
2.9.3


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

Posted by David Gibson 142 weeks ago
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