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];
> }
>
>
