Message ID | 20180111040832.28383-1-aik@ozlabs.ru |
---|---|
State | New |
Headers | show |
Series | [qemu] target/ppc: Yet another fix for KVM-HV HPTE accessors | expand |
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);
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.
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. >
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);
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(-)