On 2/14/2018 11:28 AM, Paolo Bonzini wrote:
On 13/02/2018 19:26, Joe Moriarty wrote:
The Parfait (version 2.1.0) static code analysis tool found the
following NULL pointer dereference problem.
- arch/x86/kvm/mmu.c
There is a possibility that the call to __gfn_to_rmap() can happen
with a NULL pointer given for the slot argument. This can happen
if the slot information cannot be determined from a previous call
to __gfn_to_memslot(). The base_gfn will be passed in as 0 to
gfn_to_index if no valid slot information can be obtained from a
call to __gfn_to_memslot().
How does it justify the possible NULL pointer dereference?
The fix below is incorrect anyway, since it would just get a random page
out of guest memory; please report the issue without providing a patch
if you do not understand the code well.
I can't exclude there is a race condition here that can lead to a NULL
pointer dereference. However, it should be handled by passing the
struct kvm_memory_slot all the way down to __gfn_to_rmap, i.e. adding a
new argument to gfn_to_rmap, rmap_add, mmu_set_spte, __direct_map,
try_async_pf and many more functions. That's not as easy as the patch
you posted. :)
Thanks,
Paolo
Signed-off-by: Joe Moriarty <joe.moriarty@xxxxxxxxxx>
---
arch/x86/kvm/mmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 8eca1d04aeb8..69d41b5d0948 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1239,7 +1239,7 @@ static struct kvm_rmap_head *__gfn_to_rmap(gfn_t gfn, int level,
{
unsigned long idx;
- idx = gfn_to_index(gfn, slot->base_gfn, level);
+ idx = gfn_to_index(gfn, slot ? slot->base_gfn : 0, level);
return &slot->arch.rmap[level - PT_PAGE_TABLE_LEVEL][idx];
}
Hi Paolo,
Thank you for the review. I wasn't sure if the change I proposed was
correct or not. I will take your suggestion of posting to the mailing
list instead of as a patch the next time I encounter a situation like
this again. In the meantime, I will look at doing your suggestion of
passing kvm_memory_slot down to gfn_to_rmap() and the other functions
you pointed out above for the next version of the patch.
Joe
