[bug report] KVM: Dynamically allocate "new" memslots from the get-go

[bug report] KVM: Dynamically allocate "new" memslots from the get-go

[Dan Carpenter]

Hello Sean Christopherson,

This is a semi-automatic email about new static checker warnings.

The patch 244893fa2859: "KVM: Dynamically allocate "new" memslots
from the get-go" from Dec 6, 2021, leads to the following Smatch
complaint:

    arch/x86/kvm/../../../virt/kvm/kvm_main.c:1526 kvm_prepare_memory_region()
    warn: variable dereferenced before check 'new' (see line 1509)

arch/x86/kvm/../../../virt/kvm/kvm_main.c
  1508		if (change != KVM_MR_DELETE) {
  1509			if (!(new->flags & KVM_MEM_LOG_DIRTY_PAGES))
  1510				new->dirty_bitmap = NULL;
  1511			else if (old && old->dirty_bitmap)
  1512				new->dirty_bitmap = old->dirty_bitmap;
  1513			else if (!kvm->dirty_ring_size) {
  1514				r = kvm_alloc_dirty_bitmap(new);
  1515				if (r)
  1516					return r;
  1517	
  1518				if (kvm_dirty_log_manual_protect_and_init_set(kvm))
  1519					bitmap_set(new->dirty_bitmap, 0, new->npages);
                                                   ^^^^^
  1520			}
  1521		}
  1522	
  1523		r = kvm_arch_prepare_memory_region(kvm, old, new, change);
                                                             ^^^
Lots of unchecked dereferences

  1524	
  1525		/* Free the bitmap on failure if it was allocated above. */
  1526		if (r && new && new->dirty_bitmap && old && !old->dirty_bitmap)
                         ^^^
New check for NULL.  Can this be NULL?

  1527			kvm_destroy_dirty_bitmap(new);
  1528	

regards,
dan carpenter

Re: [bug report] KVM: Dynamically allocate "new" memslots from the get-go

[Sean Christopherson]

On Wed, Dec 15, 2021, Dan Carpenter wrote:
> Hello Sean Christopherson,
> 
> This is a semi-automatic email about new static checker warnings.

These are all ok, KVM is being clever and using implicit checks on pointers in
select flows, while using explicit checks in others.

> The patch 244893fa2859: "KVM: Dynamically allocate "new" memslots
> from the get-go" from Dec 6, 2021, leads to the following Smatch
> complaint:
> 
>     arch/x86/kvm/../../../virt/kvm/kvm_main.c:1526 kvm_prepare_memory_region()
>     warn: variable dereferenced before check 'new' (see line 1509)

@new is guaranteed to be non-NULL in the !KVM_MR_DELETE case.
 
> arch/x86/kvm/../../../virt/kvm/kvm_main.c
>   1508		if (change != KVM_MR_DELETE) {
>   1509			if (!(new->flags & KVM_MEM_LOG_DIRTY_PAGES))
>   1510				new->dirty_bitmap = NULL;
>   1511			else if (old && old->dirty_bitmap)
>   1512				new->dirty_bitmap = old->dirty_bitmap;
>   1513			else if (!kvm->dirty_ring_size) {
>   1514				r = kvm_alloc_dirty_bitmap(new);
>   1515				if (r)
>   1516					return r;
>   1517	
>   1518				if (kvm_dirty_log_manual_protect_and_init_set(kvm))
>   1519					bitmap_set(new->dirty_bitmap, 0, new->npages);
>                                                    ^^^^^
>   1520			}
>   1521		}
>   1522	
>   1523		r = kvm_arch_prepare_memory_region(kvm, old, new, change);
>                                                              ^^^
> Lots of unchecked dereferences

There's no true dereference, architectures are responsible for ensuring @old and
@new are non-NULL, either via explicit checks on the pointer or implicit checks
on @change.

>   1524	
>   1525		/* Free the bitmap on failure if it was allocated above. */
>   1526		if (r && new && new->dirty_bitmap && old && !old->dirty_bitmap)
>                          ^^^
> New check for NULL.  Can this be NULL?

Yes, when change == KVM_MR_DELETE.

>   1527			kvm_destroy_dirty_bitmap(new);
>   1528	
> 
> regards,
> dan carpenter

Re: [bug report] KVM: Dynamically allocate "new" memslots from the get-go

[Dan Carpenter]

On Wed, Dec 15, 2021 at 04:34:27PM +0000, Sean Christopherson wrote:
> There's no true dereference, architectures are responsible for ensuring @old and
> @new are non-NULL, either via explicit checks on the pointer or implicit checks
> on @change.

Ah...  Okay.  Thanks.

Smatch tries to track these implicit checks within a function but the
relationships get lost at the function boundaries.  So, for example, if
there were three callers and two were non-NULL and one was NULL then
Smatch could figure it out.  But in this case there is only one caller
so the data gets squished together and the relationship between "change"
and "new" gets lost.

regards,
dan carpenter