target/ppc: Yet another fix for KVM-HV HPTE accessors

[Qemu-devel] [PATCH qemu] target/ppc: Yet another fix for KVM-HV HPTE accessors

Posted by Alexey Kardashevskiy 6 years, 3 months ago

As stated in the 1ad9f0a464fe commit log, the returned entries are not
a while PTEG. It was not a problem before 1ad9f0a464fe as it would read
a single record assuming it contains a whole PTEG but now the code tries
reading the entire PTEG and "if ((n - i) < invalid)" produces negative
values which then are converted to size_t for memset() and that throws
seg fault.

This fixes the math.

While here, fix the last @i increment as well.

Fixes: 1ad9f0a464fe "target/ppc: Fix KVM-HV HPTE accessors"
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

Record #0:
(gdb) p *hdr
$13 = {
  index = <ptr>,
  n_valid = 0x1,
  n_invalid = 0x6
}

Record #1:
(gdb) p *hdr
$18 = {
  index = <ptr>,
  n_valid = 0x2,
  n_invalid = 0x6
}


i.e. in the second iteration of the loop right before
"if ((n - i) < invalid)":
(gdb) p n
$16 = 0x8
(gdb) p i
$17 = 0x9

and @invalid becomes -1.

---
 target/ppc/kvm.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 0566af7..c2dea81 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2657,21 +2657,24 @@ void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
 
         hdr = (struct kvm_get_htab_header *)buf;
         while ((i < n) && ((char *)hdr < (buf + rc))) {
-            int invalid = hdr->n_invalid;
+            int invalid = hdr->n_invalid, valid = hdr->n_valid;
 
             if (hdr->index != (ptex + i)) {
                 hw_error("kvmppc_read_hptes: Unexpected HPTE index %"PRIu32
                          " != (%"HWADDR_PRIu" + %d", hdr->index, ptex, i);
             }
 
-            memcpy(hptes + i, hdr + 1, HASH_PTE_SIZE_64 * hdr->n_valid);
-            i += hdr->n_valid;
+            if (n - i < valid) {
+                valid = n - i;
+            }
+            memcpy(hptes + i, hdr + 1, HASH_PTE_SIZE_64 * valid);
+            i += valid;
 
             if ((n - i) < invalid) {
                 invalid = n - i;
             }
             memset(hptes + i, 0, invalid * HASH_PTE_SIZE_64);
-            i += hdr->n_invalid;
+            i += invalid;
 
             hdr = (struct kvm_get_htab_header *)
                 ((char *)(hdr + 1) + HASH_PTE_SIZE_64 * hdr->n_valid);
-- 
2.11.0

Re: [Qemu-devel] [PATCH qemu] target/ppc: Yet another fix for KVM-HV HPTE accessors

Posted by David Gibson 6 years, 3 months ago

On Thu, Jan 11, 2018 at 03:08:32PM +1100, Alexey Kardashevskiy wrote:
> As stated in the 1ad9f0a464fe commit log, the returned entries are not
> a while PTEG. It was not a problem before 1ad9f0a464fe as it would read
> a single record assuming it contains a whole PTEG but now the code tries
> reading the entire PTEG and "if ((n - i) < invalid)" produces negative
> values which then are converted to size_t for memset() and that throws
> seg fault.
> 
> This fixes the math.
> 
> While here, fix the last @i increment as well.
> 
> Fixes: 1ad9f0a464fe "target/ppc: Fix KVM-HV HPTE accessors"
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Applied, thanks.

> ---
> 
> Record #0:
> (gdb) p *hdr
> $13 = {
>   index = <ptr>,
>   n_valid = 0x1,
>   n_invalid = 0x6
> }
> 
> Record #1:
> (gdb) p *hdr
> $18 = {
>   index = <ptr>,
>   n_valid = 0x2,
>   n_invalid = 0x6
> }
> 
> 
> i.e. in the second iteration of the loop right before
> "if ((n - i) < invalid)":
> (gdb) p n
> $16 = 0x8
> (gdb) p i
> $17 = 0x9
> 
> and @invalid becomes -1.
> 
> ---
>  target/ppc/kvm.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 0566af7..c2dea81 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2657,21 +2657,24 @@ void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
>  
>          hdr = (struct kvm_get_htab_header *)buf;
>          while ((i < n) && ((char *)hdr < (buf + rc))) {
> -            int invalid = hdr->n_invalid;
> +            int invalid = hdr->n_invalid, valid = hdr->n_valid;
>  
>              if (hdr->index != (ptex + i)) {
>                  hw_error("kvmppc_read_hptes: Unexpected HPTE index %"PRIu32
>                           " != (%"HWADDR_PRIu" + %d", hdr->index, ptex, i);
>              }
>  
> -            memcpy(hptes + i, hdr + 1, HASH_PTE_SIZE_64 * hdr->n_valid);
> -            i += hdr->n_valid;
> +            if (n - i < valid) {
> +                valid = n - i;
> +            }
> +            memcpy(hptes + i, hdr + 1, HASH_PTE_SIZE_64 * valid);
> +            i += valid;
>  
>              if ((n - i) < invalid) {
>                  invalid = n - i;
>              }
>              memset(hptes + i, 0, invalid * HASH_PTE_SIZE_64);
> -            i += hdr->n_invalid;
> +            i += invalid;
>  
>              hdr = (struct kvm_get_htab_header *)
>                  ((char *)(hdr + 1) + HASH_PTE_SIZE_64 * hdr->n_valid);

-- 
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

Re: [Qemu-devel] [PATCH qemu] target/ppc: Yet another fix for KVM-HV HPTE accessors

Posted by Mark Cave-Ayland 6 years, 3 months ago

On 11/01/18 04:08, Alexey Kardashevskiy wrote:

> As stated in the 1ad9f0a464fe commit log, the returned entries are not
> a while PTEG. It was not a problem before 1ad9f0a464fe as it would read

s/while/whole/?

> a single record assuming it contains a whole PTEG but now the code tries
> reading the entire PTEG and "if ((n - i) < invalid)" produces negative
> values which then are converted to size_t for memset() and that throws
> seg fault.
> 
> This fixes the math.
> 
> While here, fix the last @i increment as well.
> 
> Fixes: 1ad9f0a464fe "target/ppc: Fix KVM-HV HPTE accessors"
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> Record #0:
> (gdb) p *hdr
> $13 = {
>    index = <ptr>,
>    n_valid = 0x1,
>    n_invalid = 0x6
> }
> 
> Record #1:
> (gdb) p *hdr
> $18 = {
>    index = <ptr>,
>    n_valid = 0x2,
>    n_invalid = 0x6
> }
> 
> 
> i.e. in the second iteration of the loop right before
> "if ((n - i) < invalid)":
> (gdb) p n
> $16 = 0x8
> (gdb) p i
> $17 = 0x9
> 
> and @invalid becomes -1.
> 
> ---
>   target/ppc/kvm.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 0566af7..c2dea81 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2657,21 +2657,24 @@ void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
>   
>           hdr = (struct kvm_get_htab_header *)buf;
>           while ((i < n) && ((char *)hdr < (buf + rc))) {
> -            int invalid = hdr->n_invalid;
> +            int invalid = hdr->n_invalid, valid = hdr->n_valid;
>   
>               if (hdr->index != (ptex + i)) {
>                   hw_error("kvmppc_read_hptes: Unexpected HPTE index %"PRIu32
>                            " != (%"HWADDR_PRIu" + %d", hdr->index, ptex, i);
>               }
>   
> -            memcpy(hptes + i, hdr + 1, HASH_PTE_SIZE_64 * hdr->n_valid);
> -            i += hdr->n_valid;
> +            if (n - i < valid) {
> +                valid = n - i;
> +            }
> +            memcpy(hptes + i, hdr + 1, HASH_PTE_SIZE_64 * valid);
> +            i += valid;
>   
>               if ((n - i) < invalid) {
>                   invalid = n - i;
>               }
>               memset(hptes + i, 0, invalid * HASH_PTE_SIZE_64);
> -            i += hdr->n_invalid;
> +            i += invalid;
>   
>               hdr = (struct kvm_get_htab_header *)
>                   ((char *)(hdr + 1) + HASH_PTE_SIZE_64 * hdr->n_valid);
> 


ATB,

Mark.

Re: [Qemu-devel] [PATCH qemu] target/ppc: Yet another fix for KVM-HV HPTE accessors

Posted by David Gibson 6 years, 3 months ago

On Thu, Jan 11, 2018 at 08:06:13AM +0000, Mark Cave-Ayland wrote:
> On 11/01/18 04:08, Alexey Kardashevskiy wrote:
> 
> > As stated in the 1ad9f0a464fe commit log, the returned entries are not
> > a while PTEG. It was not a problem before 1ad9f0a464fe as it would read
> 
> s/while/whole/?

I already merged, so I adjusted this in my tree.  Come to that
although the fix is correct, I don't think the current message
explains it very well.  I'll think about adjusting it.

> 
> > a single record assuming it contains a whole PTEG but now the code tries
> > reading the entire PTEG and "if ((n - i) < invalid)" produces negative
> > values which then are converted to size_t for memset() and that throws
> > seg fault.
> > 
> > This fixes the math.
> > 
> > While here, fix the last @i increment as well.
> > 
> > Fixes: 1ad9f0a464fe "target/ppc: Fix KVM-HV HPTE accessors"
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> > 
> > Record #0:
> > (gdb) p *hdr
> > $13 = {
> >    index = <ptr>,
> >    n_valid = 0x1,
> >    n_invalid = 0x6
> > }
> > 
> > Record #1:
> > (gdb) p *hdr
> > $18 = {
> >    index = <ptr>,
> >    n_valid = 0x2,
> >    n_invalid = 0x6
> > }
> > 
> > 
> > i.e. in the second iteration of the loop right before
> > "if ((n - i) < invalid)":
> > (gdb) p n
> > $16 = 0x8
> > (gdb) p i
> > $17 = 0x9
> > 
> > and @invalid becomes -1.
> > 
> > ---
> >   target/ppc/kvm.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 0566af7..c2dea81 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -2657,21 +2657,24 @@ void kvmppc_read_hptes(ppc_hash_pte64_t *hptes, hwaddr ptex, int n)
> >           hdr = (struct kvm_get_htab_header *)buf;
> >           while ((i < n) && ((char *)hdr < (buf + rc))) {
> > -            int invalid = hdr->n_invalid;
> > +            int invalid = hdr->n_invalid, valid = hdr->n_valid;
> >               if (hdr->index != (ptex + i)) {
> >                   hw_error("kvmppc_read_hptes: Unexpected HPTE index %"PRIu32
> >                            " != (%"HWADDR_PRIu" + %d", hdr->index, ptex, i);
> >               }
> > -            memcpy(hptes + i, hdr + 1, HASH_PTE_SIZE_64 * hdr->n_valid);
> > -            i += hdr->n_valid;
> > +            if (n - i < valid) {
> > +                valid = n - i;
> > +            }
> > +            memcpy(hptes + i, hdr + 1, HASH_PTE_SIZE_64 * valid);
> > +            i += valid;
> >               if ((n - i) < invalid) {
> >                   invalid = n - i;
> >               }
> >               memset(hptes + i, 0, invalid * HASH_PTE_SIZE_64);
> > -            i += hdr->n_invalid;
> > +            i += invalid;
> >               hdr = (struct kvm_get_htab_header *)
> >                   ((char *)(hdr + 1) + HASH_PTE_SIZE_64 * hdr->n_valid);
> > 
> 
> 
> ATB,
> 
> Mark.
> 

-- 
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