[PATCH v1 0/1] Parfait changes

[PATCH v1 0/1] Parfait changes

[Joe Moriarty]

The following patch(s) are bugs found by the static compiler
'Parfait'.  Care was taken to make sure false positive results
were removed from this patchset.

Parfait Overview
================

https://labs.oracle.com/pls/apex/f?p=labs:49:::::P49_PROJECT_ID:13

v1:
Initial release

Joe Moriarty (1):
  KVM: X86: NULL pointer dereference [null-pointer-deref] (CWE 476)
    problem

 arch/x86/kvm/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.15.0

[PATCH v1 1/1] KVM: X86: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

[Joe Moriarty]

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().

Signed-off-by: Joe Moriarty <joe.moriarty@oracle.com>
---
 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];
 }
 
-- 
2.15.0

Re: [PATCH v1 1/1] KVM: X86: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

[Paolo Bonzini]

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@oracle.com>
> ---
>  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];
>  }
>  
> 

Re: [PATCH v1 1/1] KVM: X86: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

[Joe Moriarty]

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@oracle.com>
>> ---
>>   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

Re: [PATCH v1 1/1] KVM: X86: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

[Paolo Bonzini]

On 14/02/2018 17:55, Joe Moriarty wrote:
>>
> 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.

It's not easy, but I can send you a box of beers if you get round to it.
 Note that I'm still not sure how the NULL pointer dereference can
happen, and you didn't include more output from your tool, so you might
be wasting your time after all...

Anyway, I would start basically by mapping the paths from try_async_pf's
callers to mmu_set_spte and from there to rmap_add.

On the other hand, in the rmap_remove path, you probably should just
exit immediately if slot is NULL.  (Guangrong, do you have any idea why
we don't zap SPTEs in kvm_arch_free_memslot?)

Paolo

Re: [PATCH v1 1/1] KVM: X86: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

[Joe Moriarty]

On 2/14/2018 12:33 PM, Paolo Bonzini wrote:
> On 14/02/2018 17:55, Joe Moriarty wrote:
>>>
>> 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.
> 
> It's not easy, but I can send you a box of beers if you get round to it.
>   Note that I'm still not sure how the NULL pointer dereference can
> happen, and you didn't include more output from your tool, so you might
> be wasting your time after all...
> 
> Anyway, I would start basically by mapping the paths from try_async_pf's
> callers to mmu_set_spte and from there to rmap_add.
> 
> On the other hand, in the rmap_remove path, you probably should just
> exit immediately if slot is NULL.  (Guangrong, do you have any idea why
> we don't zap SPTEs in kvm_arch_free_memslot?)
> 
> Paolo
> 
The crux of the problem is the possible return of NULL from 
search_memslots() located at line 950 in include/linux/kvm_host.h.  This 
is called by __gfn_to_memslot().  I would assume this is a case of this 
is never going to happen.  I say this because the kernel would panic 
today with a NULL pointer dereference at line 1254 of arch/x86/kvm/mmu.c 
if it did happen.

I was thinking a BUG_ON() would be more appropriate at the end of 
search_memslots() before the "return NULL".  That is if my 
aforementioned assumption is correct about the code path.  If not, then 
continue on with the suggested fix of passing down kvm_memory_slot to 
gfn_to_rmap().

Let me know which solution you would like.
Joe

Re: [PATCH v1 1/1] KVM: X86: NULL pointer dereference [null-pointer-deref] (CWE 476) problem

[Paolo Bonzini]

----- Original Message -----
> From: "Joe Moriarty" <joe.moriarty@oracle.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>, rkrcmar@redhat.com, kvm@vger.kernel.org, "Xiao Guangrong"
> <xiaoguangrong@tencent.com>
> Sent: Wednesday, February 14, 2018 7:14:09 PM
> Subject: Re: [PATCH v1 1/1] KVM: X86: NULL pointer dereference [null-pointer-deref] (CWE 476) problem
> 
> On 2/14/2018 12:33 PM, Paolo Bonzini wrote:
> > On 14/02/2018 17:55, Joe Moriarty wrote:
> >>>
> >> 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.
> > 
> > It's not easy, but I can send you a box of beers if you get round to it.
> >   Note that I'm still not sure how the NULL pointer dereference can
> > happen, and you didn't include more output from your tool, so you might
> > be wasting your time after all...
> > 
> > Anyway, I would start basically by mapping the paths from try_async_pf's
> > callers to mmu_set_spte and from there to rmap_add.
> > 
> > On the other hand, in the rmap_remove path, you probably should just
> > exit immediately if slot is NULL.  (Guangrong, do you have any idea why
> > we don't zap SPTEs in kvm_arch_free_memslot?)
> 
> The crux of the problem is the possible return of NULL from
> search_memslots() located at line 950 in include/linux/kvm_host.h.  This
> is called by __gfn_to_memslot().  I would assume this is a case of this
> is never going to happen.  I say this because the kernel would panic
> today with a NULL pointer dereference at line 1254 of arch/x86/kvm/mmu.c
> if it did happen.

I cannot really rule out that it won't happen.  I didn't have in mind
that NULL-pointer dereference, but it's always looked racy to me---I've
never gotten round to fix it because I wasn't sure about the right thing
to do on the rmap_remove path.

> I was thinking a BUG_ON() would be more appropriate at the end of
> search_memslots() before the "return NULL".

No, this is probably too much.  The right fix is to pass down the memslot,
thus avoiding gfn_to_rmap in favor of __gfn_to_rmap.

Paolo