Hello,
I have found a possible out of bounds reading in
arch/powerpc/kvm/book3s_64_mmu.c (kvmppc_mmu_book3s_64_xlate
function). pteg[] array could be accessed twice using the i variable
after the for iteration. What happens is that in the last iteration
the i index is incremented to 16, checked (i<16) then confirmed
exiting the loop.
277 for (i=0; i<16; i+=2) { ...
Later there are reading attempts to the pteg last elements, but using
again the already incremented i (16).
303 v = be64_to_cpu(pteg[i]); /* pteg[16] */
304 r = be64_to_cpu(pteg[i+1]); /* pteg[17] */
I really don't know if the for lace will somehow iterate until i is
16, anyway I think that the last readings must be using a defined max
len/index or another more clear method.
Eg.
v = be64_to_cpu(pteg[PTEG_LEN - 2]);
r = be64_to_cpu(pteg[PTEG_LEN - 1]);
Or just.
v = be64_to_cpu(pteg[14]);
r = be64_to_cpu(pteg[15]);
----------------------------
I found in the same file a variable that is not used.
380 struct kvmppc_vcpu_book3s *vcpu_book3s;
...
387 vcpu_book3s = to_book3s(vcpu);
-----------------------------
A question, the kvmppc_mmu_book3s_64_init function is accessed by
unconventional way? Because I have not found any calling to it.
If something that I wrote is correct please tell me if I could send the patch.
--
Regards,
Geyslan G. Bem
hackingbits.com
On Sun, Nov 29, 2015 at 05:14:03PM -0300, Geyslan Gregório Bem wrote:
Hello,
I have found a possible out of bounds reading in
arch/powerpc/kvm/book3s_64_mmu.c (kvmppc_mmu_book3s_64_xlate
function). pteg[] array could be accessed twice using the i variable
after the for iteration. What happens is that in the last iteration
the i index is incremented to 16, checked (i<16) then confirmed
exiting the loop.
277 for (i=0; i<16; i+=2) { ...
Later there are reading attempts to the pteg last elements, but using
again the already incremented i (16).
303 v = be64_to_cpu(pteg[i]); /* pteg[16] */
304 r = be64_to_cpu(pteg[i+1]); /* pteg[17] */
Was it some automated tool that came up with this?
There is actually no problem because the accesses outside the loop are
only done if the 'found' variable is true; 'found' is initialized to
false and only ever set to true inside the loop just before a break
statement. Thus there is a correlation between the value of 'i' and
the value of 'found' -- if 'found' is true then we know 'i' is less
than 16.
I really don't know if the for lace will somehow iterate until i is
16, anyway I think that the last readings must be using a defined max
len/index or another more clear method.
I think it's perfectly clear to a human programmer, though some tools
(such as gcc) struggle with this kind of correlation between
variables. That's why I asked whether your report was based on the
output from some tool.
Eg.
v = be64_to_cpu(pteg[PTEG_LEN - 2]);
r = be64_to_cpu(pteg[PTEG_LEN - 1]);
Or just.
v = be64_to_cpu(pteg[14]);
r = be64_to_cpu(pteg[15]);
Either of those options would cause the code to malfunction.
I found in the same file a variable that is not used.
380 struct kvmppc_vcpu_book3s *vcpu_book3s;
...
387 vcpu_book3s = to_book3s(vcpu);
True. It could be removed.
A question, the kvmppc_mmu_book3s_64_init function is accessed by
unconventional way? Because I have not found any calling to it.
Try arch/powerpc/kvm/book3s_pr.c line 410:
kvmppc_mmu_book3s_64_init(vcpu);
Grep (or git grep) is your friend.
Paul.
2015-11-29 18:33 GMT-03:00 Paul Mackerras [off-list ref]:
On Sun, Nov 29, 2015 at 05:14:03PM -0300, Geyslan Greg=C3=B3rio Bem wrote=
:
quoted
Hello,
I have found a possible out of bounds reading in
arch/powerpc/kvm/book3s_64_mmu.c (kvmppc_mmu_book3s_64_xlate
function). pteg[] array could be accessed twice using the i variable
after the for iteration. What happens is that in the last iteration
the i index is incremented to 16, checked (i<16) then confirmed
exiting the loop.
277 for (i=3D0; i<16; i+=3D2) { ...
Later there are reading attempts to the pteg last elements, but using
again the already incremented i (16).
303 v =3D be64_to_cpu(pteg[i]); /* pteg[16] */
304 r =3D be64_to_cpu(pteg[i+1]); /* pteg[17] */
Was it some automated tool that came up with this?
Yep, cppcheck. As I'm still not engaged to a specific area in the
kernel, just trying to help with automated catches.
There is actually no problem because the accesses outside the loop are
only done if the 'found' variable is true; 'found' is initialized to
false and only ever set to true inside the loop just before a break
statement. Thus there is a correlation between the value of 'i' and
the value of 'found' -- if 'found' is true then we know 'i' is less
than 16.
I figured it out after your explanation.
quoted
I really don't know if the for lace will somehow iterate until i is
16, anyway I think that the last readings must be using a defined max
len/index or another more clear method.
I think it's perfectly clear to a human programmer, though some tools
(such as gcc) struggle with this kind of correlation between
variables. That's why I asked whether your report was based on the
output from some tool.
quoted
Eg.
v =3D be64_to_cpu(pteg[PTEG_LEN - 2]);
r =3D be64_to_cpu(pteg[PTEG_LEN - 1]);
Or just.
v =3D be64_to_cpu(pteg[14]);
r =3D be64_to_cpu(pteg[15]);
Either of those options would cause the code to malfunction.
Yep, I understand now that v and r get the found ones. So i is needed.
quoted
I found in the same file a variable that is not used.
380 struct kvmppc_vcpu_book3s *vcpu_book3s;
...
387 vcpu_book3s =3D to_book3s(vcpu);
True. It could be removed.
I'll make a patch for that.
quoted
A question, the kvmppc_mmu_book3s_64_init function is accessed by
unconventional way? Because I have not found any calling to it.
Try arch/powerpc/kvm/book3s_pr.c line 410:
kvmppc_mmu_book3s_64_init(vcpu);
Grep (or git grep) is your friend.
Hmm, indeed.
Paul.
Thank you, Paul. If you have some other changes in progress let me know.
--=20
Regards,
Geyslan G. Bem
hackingbits.com