[PATCH 0/7] KVM: Fix memslot use-after-free bug

[PATCH 0/7] KVM: Fix memslot use-after-free bug

[Sean Christopherson]

Fix a bug introduced by dynamic memslot allocation where the LRU slot can
become invalid and lead to a out-of-bounds/use-after-free scenario.

The patch is different that what Qian has already tested, but I was able
to reproduce the bad behavior by enhancing the set memory region selftest,
i.e. I'm relatively confident the bug is fixed.

Patches 2-6 are a variety of selftest cleanup, with the aforementioned
set memory region enhancement coming in patch 7.

Note, I couldn't get the selftest to fail outright or with KASAN, but was
able to hit a WARN_ON an invalid slot 100% of the time (without the fix,
obviously).

Regarding s390, I played around a bit with merging gfn_to_memslot_approx()
into search_memslots().  Code wise it's trivial since they're basically
identical, but doing so increases the code footprint of search_memslots()
on x86 by 30 bytes, so I ended up abandoning the effort.

Sean Christopherson (7):
  KVM: Fix out of range accesses to memslots
  KVM: selftests: Fix cosmetic copy-paste error in vm_mem_region_move()
  KVM: selftests: Take vcpu pointer instead of id in vm_vcpu_rm()
  KVM: selftests: Add helpers to consolidate open coded list operations
  KVM: selftests: Add util to delete memory region
  KVM: selftests: Expose the primary memslot number to tests
  KVM: selftests: Add "delete" testcase to set_memory_region_test

 arch/s390/kvm/kvm-s390.c                      |   3 +
 include/linux/kvm_host.h                      |   3 +
 .../testing/selftests/kvm/include/kvm_util.h  |   3 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 139 ++++++++++--------
 .../kvm/x86_64/set_memory_region_test.c       | 122 +++++++++++++--
 virt/kvm/kvm_main.c                           |   3 +
 6 files changed, 201 insertions(+), 72 deletions(-)

-- 
2.24.1

[PATCH 1/7] KVM: Fix out of range accesses to memslots

[Sean Christopherson]

Reset the LRU slot if it becomes invalid when deleting a memslot to fix
an out-of-bounds/use-after-free access when searching through memslots.

Explicitly check for there being no used slots in search_memslots(), and
in the caller of s390's approximation variant.

Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on number of used slots")
Reported-by: Qian Cai <cai@lca.pw>
Cc: Peter Xu <peterx@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/s390/kvm/kvm-s390.c | 3 +++
 include/linux/kvm_host.h | 3 +++
 virt/kvm/kvm_main.c      | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 807ed6d722dd..cb15fdda1fee 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args,
 	struct kvm_memslots *slots = kvm_memslots(kvm);
 	struct kvm_memory_slot *ms;
 
+	if (unlikely(!slots->used_slots))
+		return 0;
+
 	cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn);
 	ms = gfn_to_memslot(kvm, cur_gfn);
 	args->count = 0;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 35bc52e187a2..b19dee4ed7d9 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn)
 	int slot = atomic_read(&slots->lru_slot);
 	struct kvm_memory_slot *memslots = slots->memslots;
 
+	if (unlikely(!slots->used_slots))
+		return NULL;
+
 	if (gfn >= memslots[slot].base_gfn &&
 	    gfn < memslots[slot].base_gfn + memslots[slot].npages)
 		return &memslots[slot];
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 28eae681859f..f744bc603c53 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct kvm_memslots *slots,
 
 	slots->used_slots--;
 
+	if (atomic_read(&slots->lru_slot) >= slots->used_slots)
+		atomic_set(&slots->lru_slot, 0);
+
 	for (i = slots->id_to_index[memslot->id]; i < slots->used_slots; i++) {
 		mslots[i] = mslots[i + 1];
 		slots->id_to_index[mslots[i].id] = i;
-- 
2.24.1

Re: [PATCH 1/7] KVM: Fix out of range accesses to memslots

[Peter Xu]

On Fri, Mar 20, 2020 at 01:55:40PM -0700, Sean Christopherson wrote:
> Reset the LRU slot if it becomes invalid when deleting a memslot to fix
> an out-of-bounds/use-after-free access when searching through memslots.
> 
> Explicitly check for there being no used slots in search_memslots(), and
> in the caller of s390's approximation variant.
> 
> Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on number of used slots")
> Reported-by: Qian Cai <cai@lca.pw>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 3 +++
>  include/linux/kvm_host.h | 3 +++
>  virt/kvm/kvm_main.c      | 3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 807ed6d722dd..cb15fdda1fee 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args,
>  	struct kvm_memslots *slots = kvm_memslots(kvm);
>  	struct kvm_memory_slot *ms;
>  
> +	if (unlikely(!slots->used_slots))
> +		return 0;
> +
>  	cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn);
>  	ms = gfn_to_memslot(kvm, cur_gfn);
>  	args->count = 0;
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 35bc52e187a2..b19dee4ed7d9 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn)
>  	int slot = atomic_read(&slots->lru_slot);
>  	struct kvm_memory_slot *memslots = slots->memslots;
>  
> +	if (unlikely(!slots->used_slots))
> +		return NULL;
> +
>  	if (gfn >= memslots[slot].base_gfn &&
>  	    gfn < memslots[slot].base_gfn + memslots[slot].npages)
>  		return &memslots[slot];
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 28eae681859f..f744bc603c53 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct kvm_memslots *slots,
>  
>  	slots->used_slots--;
>  
> +	if (atomic_read(&slots->lru_slot) >= slots->used_slots)
> +		atomic_set(&slots->lru_slot, 0);

Nit: could we drop the atomic ops?  The "slots" is still only used in
the current thread before the rcu assignment, so iirc it's fine (and
RCU assignment should have a mem barrier when needed anyway).

I thought resetting lru_slot to zero would be good enough when
duplicating the slots... however if we want to do finer grained reset,
maybe even better to reset also those invalidated ones (since we know
this information)?

   	if (slots->lru_slot >= slots->id_to_index[memslot->id])
   		slots->lru_slot = 0;
  
Thanks,

> +
>  	for (i = slots->id_to_index[memslot->id]; i < slots->used_slots; i++) {
>  		mslots[i] = mslots[i + 1];
>  		slots->id_to_index[mslots[i].id] = i;
> -- 
> 2.24.1
> 

-- 
Peter Xu

Re: [PATCH 1/7] KVM: Fix out of range accesses to memslots

[Sean Christopherson]

On Fri, Mar 20, 2020 at 06:17:08PM -0400, Peter Xu wrote:
> On Fri, Mar 20, 2020 at 01:55:40PM -0700, Sean Christopherson wrote:
> > Reset the LRU slot if it becomes invalid when deleting a memslot to fix
> > an out-of-bounds/use-after-free access when searching through memslots.
> > 
> > Explicitly check for there being no used slots in search_memslots(), and
> > in the caller of s390's approximation variant.
> > 
> > Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on number of used slots")
> > Reported-by: Qian Cai <cai@lca.pw>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/s390/kvm/kvm-s390.c | 3 +++
> >  include/linux/kvm_host.h | 3 +++
> >  virt/kvm/kvm_main.c      | 3 +++
> >  3 files changed, 9 insertions(+)
> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index 807ed6d722dd..cb15fdda1fee 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args,
> >  	struct kvm_memslots *slots = kvm_memslots(kvm);
> >  	struct kvm_memory_slot *ms;
> >  
> > +	if (unlikely(!slots->used_slots))
> > +		return 0;
> > +
> >  	cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn);
> >  	ms = gfn_to_memslot(kvm, cur_gfn);
> >  	args->count = 0;
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 35bc52e187a2..b19dee4ed7d9 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn)
> >  	int slot = atomic_read(&slots->lru_slot);
> >  	struct kvm_memory_slot *memslots = slots->memslots;
> >  
> > +	if (unlikely(!slots->used_slots))
> > +		return NULL;
> > +
> >  	if (gfn >= memslots[slot].base_gfn &&
> >  	    gfn < memslots[slot].base_gfn + memslots[slot].npages)
> >  		return &memslots[slot];
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 28eae681859f..f744bc603c53 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct kvm_memslots *slots,
> >  
> >  	slots->used_slots--;
> >  
> > +	if (atomic_read(&slots->lru_slot) >= slots->used_slots)
> > +		atomic_set(&slots->lru_slot, 0);
> 
> Nit: could we drop the atomic ops?  The "slots" is still only used in
> the current thread before the rcu assignment, so iirc it's fine (and
> RCU assignment should have a mem barrier when needed anyway).

No, atomic_t wraps an int inside a struct to prevent direct usage, e.g.

virt/kvm/kvm_main.c: In function ‘kvm_memslot_delete’:
virt/kvm/kvm_main.c:885:22: error: invalid operands to binary >= (have ‘atomic_t {aka struct <anonymous>}’ and ‘int’)
  if (slots->lru_slot >= slots->used_slots)
                      ^
virt/kvm/kvm_main.c:886:19: error: incompatible types when assigning to type ‘atomic_t {aka struct <anonymous>}’ from type ‘int’
   slots->lru_slot = 0;


Buy yeah, the compiler barrier associated with atomic_read() isn't
necessary.

> I thought resetting lru_slot to zero would be good enough when
> duplicating the slots... however if we want to do finer grained reset,
> maybe even better to reset also those invalidated ones (since we know
> this information)?
> 
>    	if (slots->lru_slot >= slots->id_to_index[memslot->id])
>    		slots->lru_slot = 0;

I'd prefer to go with the more obviously correct version.  This code will
rarely trigger, e.g. larger slots have lower indices and are more likely to
be the LRU (by virtue of being the biggest), and dynamic memslot deletion
is usually for relatively small ranges that are being remapped by the guest.

> Thanks,
> 
> > +
> >  	for (i = slots->id_to_index[memslot->id]; i < slots->used_slots; i++) {
> >  		mslots[i] = mslots[i + 1];
> >  		slots->id_to_index[mslots[i].id] = i;
> > -- 
> > 2.24.1
> > 
> 
> -- 
> Peter Xu
> 

Re: [PATCH 1/7] KVM: Fix out of range accesses to memslots

[Peter Xu]

On Fri, Mar 20, 2020 at 03:40:41PM -0700, Sean Christopherson wrote:
> On Fri, Mar 20, 2020 at 06:17:08PM -0400, Peter Xu wrote:
> > On Fri, Mar 20, 2020 at 01:55:40PM -0700, Sean Christopherson wrote:
> > > Reset the LRU slot if it becomes invalid when deleting a memslot to fix
> > > an out-of-bounds/use-after-free access when searching through memslots.
> > > 
> > > Explicitly check for there being no used slots in search_memslots(), and
> > > in the caller of s390's approximation variant.
> > > 
> > > Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on number of used slots")
> > > Reported-by: Qian Cai <cai@lca.pw>
> > > Cc: Peter Xu <peterx@redhat.com>
> > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > > ---
> > >  arch/s390/kvm/kvm-s390.c | 3 +++
> > >  include/linux/kvm_host.h | 3 +++
> > >  virt/kvm/kvm_main.c      | 3 +++
> > >  3 files changed, 9 insertions(+)
> > > 
> > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > > index 807ed6d722dd..cb15fdda1fee 100644
> > > --- a/arch/s390/kvm/kvm-s390.c
> > > +++ b/arch/s390/kvm/kvm-s390.c
> > > @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args,
> > >  	struct kvm_memslots *slots = kvm_memslots(kvm);
> > >  	struct kvm_memory_slot *ms;
> > >  
> > > +	if (unlikely(!slots->used_slots))
> > > +		return 0;
> > > +
> > >  	cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn);
> > >  	ms = gfn_to_memslot(kvm, cur_gfn);
> > >  	args->count = 0;
> > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > index 35bc52e187a2..b19dee4ed7d9 100644
> > > --- a/include/linux/kvm_host.h
> > > +++ b/include/linux/kvm_host.h
> > > @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn)
> > >  	int slot = atomic_read(&slots->lru_slot);
> > >  	struct kvm_memory_slot *memslots = slots->memslots;
> > >  
> > > +	if (unlikely(!slots->used_slots))
> > > +		return NULL;
> > > +
> > >  	if (gfn >= memslots[slot].base_gfn &&
> > >  	    gfn < memslots[slot].base_gfn + memslots[slot].npages)
> > >  		return &memslots[slot];
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 28eae681859f..f744bc603c53 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct kvm_memslots *slots,
> > >  
> > >  	slots->used_slots--;
> > >  
> > > +	if (atomic_read(&slots->lru_slot) >= slots->used_slots)
> > > +		atomic_set(&slots->lru_slot, 0);
> > 
> > Nit: could we drop the atomic ops?  The "slots" is still only used in
> > the current thread before the rcu assignment, so iirc it's fine (and
> > RCU assignment should have a mem barrier when needed anyway).
> 
> No, atomic_t wraps an int inside a struct to prevent direct usage, e.g.
> 
> virt/kvm/kvm_main.c: In function ‘kvm_memslot_delete’:
> virt/kvm/kvm_main.c:885:22: error: invalid operands to binary >= (have ‘atomic_t {aka struct <anonymous>}’ and ‘int’)
>   if (slots->lru_slot >= slots->used_slots)
>                       ^
> virt/kvm/kvm_main.c:886:19: error: incompatible types when assigning to type ‘atomic_t {aka struct <anonymous>}’ from type ‘int’
>    slots->lru_slot = 0;
> 
> 
> Buy yeah, the compiler barrier associated with atomic_read() isn't
> necessary.

Oops, sorry. I was obviously thinking about QEMU's atomic helpers.

> 
> > I thought resetting lru_slot to zero would be good enough when
> > duplicating the slots... however if we want to do finer grained reset,
> > maybe even better to reset also those invalidated ones (since we know
> > this information)?
> > 
> >    	if (slots->lru_slot >= slots->id_to_index[memslot->id])
> >    		slots->lru_slot = 0;
> 
> I'd prefer to go with the more obviously correct version.  This code will
> rarely trigger, e.g. larger slots have lower indices and are more likely to
> be the LRU (by virtue of being the biggest), and dynamic memslot deletion
> is usually for relatively small ranges that are being remapped by the guest.

IMHO the more obvious correct version could be unconditionally setting
lru_slot to something invalid (e.g. -1) in kvm_dup_memslots(), then in
the two search_memslots() skip the cache if lru_slot is invalid.
That's IMHO easier to understand than the "!slots->used_slots" checks.
But I've no strong opinion.

Thanks,

-- 
Peter Xu

Re: [PATCH 1/7] KVM: Fix out of range accesses to memslots

[Sean Christopherson]

On Fri, Mar 20, 2020 at 06:58:02PM -0400, Peter Xu wrote:
> On Fri, Mar 20, 2020 at 03:40:41PM -0700, Sean Christopherson wrote:
 > > I thought resetting lru_slot to zero would be good enough when
> > > duplicating the slots... however if we want to do finer grained reset,
> > > maybe even better to reset also those invalidated ones (since we know
> > > this information)?
> > > 
> > >    	if (slots->lru_slot >= slots->id_to_index[memslot->id])
> > >    		slots->lru_slot = 0;
> > 
> > I'd prefer to go with the more obviously correct version.  This code will
> > rarely trigger, e.g. larger slots have lower indices and are more likely to
> > be the LRU (by virtue of being the biggest), and dynamic memslot deletion
> > is usually for relatively small ranges that are being remapped by the guest.
> 
> IMHO the more obvious correct version could be unconditionally setting
> lru_slot to something invalid (e.g. -1) in kvm_dup_memslots(), then in
> the two search_memslots() skip the cache if lru_slot is invalid.
> That's IMHO easier to understand than the "!slots->used_slots" checks.
> But I've no strong opinion.

We'd still need the !slots->used_slots check.  I considered adding an
explicit check on @slot for the cache lookup, e.g.

static inline struct kvm_memory_slot *
search_memslots(struct kvm_memslots *slots, gfn_t gfn)
{
	int start = 0, end = slots->used_slots;
	int slot = atomic_read(&slots->lru_slot);
	struct kvm_memory_slot *memslots = slots->memslots;

	if (likely(slot < slots->used_slots) &&
	    gfn >= memslots[slot].base_gfn &&
	    gfn < memslots[slot].base_gfn + memslots[slot].npages)
		return &memslots[slot];


But then the back half of the function still ends up with an out-of-bounds
bug.  E.g. if used_slots == 0, then start==0, and memslots[start].base_gfn
accesses garbage.

	while (start < end) {
		slot = start + (end - start) / 2;

		if (gfn >= memslots[slot].base_gfn)
			end = slot;
		else
			start = slot + 1;
	}

	if (gfn >= memslots[start].base_gfn &&
	    gfn < memslots[start].base_gfn + memslots[start].npages) {
		atomic_set(&slots->lru_slot, start);
		return &memslots[start];
	}

	return NULL;
}

I also didn't want to invalidate the lru_slot any more than is necessary,
not that I'd expect it to make a noticeable difference even in a
pathalogical scenario, but it seemed prudent.

Re: [PATCH 1/7] KVM: Fix out of range accesses to memslots

[Christian Borntraeger]

On 20.03.20 21:55, Sean Christopherson wrote:
> Reset the LRU slot if it becomes invalid when deleting a memslot to fix
> an out-of-bounds/use-after-free access when searching through memslots.
> 
> Explicitly check for there being no used slots in search_memslots(), and
> in the caller of s390's approximation variant.
> 
> Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on number of used slots")
> Reported-by: Qian Cai <cai@lca.pw>
> Cc: Peter Xu <peterx@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/s390/kvm/kvm-s390.c | 3 +++
>  include/linux/kvm_host.h | 3 +++
>  virt/kvm/kvm_main.c      | 3 +++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 807ed6d722dd..cb15fdda1fee 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args,

Adding Claudio, but
>  	struct kvm_memslots *slots = kvm_memslots(kvm);
>  	struct kvm_memory_slot *ms;
>  
> +	if (unlikely(!slots->used_slots))
> +		return 0;
> +

this looks sane and like the right fix.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>

>  	cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn);
>  	ms = gfn_to_memslot(kvm, cur_gfn);
>  	args->count = 0;
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 35bc52e187a2..b19dee4ed7d9 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots, gfn_t gfn)
>  	int slot = atomic_read(&slots->lru_slot);
>  	struct kvm_memory_slot *memslots = slots->memslots;
>  
> +	if (unlikely(!slots->used_slots))
> +		return NULL;
> +
>  	if (gfn >= memslots[slot].base_gfn &&
>  	    gfn < memslots[slot].base_gfn + memslots[slot].npages)
>  		return &memslots[slot];
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 28eae681859f..f744bc603c53 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct kvm_memslots *slots,
>  
>  	slots->used_slots--;
>  
> +	if (atomic_read(&slots->lru_slot) >= slots->used_slots)
> +		atomic_set(&slots->lru_slot, 0);
> +
>  	for (i = slots->id_to_index[memslot->id]; i < slots->used_slots; i++) {
>  		mslots[i] = mslots[i + 1];
>  		slots->id_to_index[mslots[i].id] = i;
> 

Re: [PATCH 1/7] KVM: Fix out of range accesses to memslots

[Claudio Imbrenda]

On Tue, 24 Mar 2020 08:12:59 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 20.03.20 21:55, Sean Christopherson wrote:
> > Reset the LRU slot if it becomes invalid when deleting a memslot to
> > fix an out-of-bounds/use-after-free access when searching through
> > memslots.
> > 
> > Explicitly check for there being no used slots in
> > search_memslots(), and in the caller of s390's approximation
> > variant.
> > 
> > Fixes: 36947254e5f9 ("KVM: Dynamically size memslot array based on
> > number of used slots") Reported-by: Qian Cai <cai@lca.pw>
> > Cc: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  arch/s390/kvm/kvm-s390.c | 3 +++
> >  include/linux/kvm_host.h | 3 +++
> >  virt/kvm/kvm_main.c      | 3 +++
> >  3 files changed, 9 insertions(+)
> > 
> > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> > index 807ed6d722dd..cb15fdda1fee 100644
> > --- a/arch/s390/kvm/kvm-s390.c
> > +++ b/arch/s390/kvm/kvm-s390.c
> > @@ -2002,6 +2002,9 @@ static int kvm_s390_get_cmma(struct kvm *kvm,
> > struct kvm_s390_cmma_log *args,  
> 
> Adding Claudio, but
> >  	struct kvm_memslots *slots = kvm_memslots(kvm);
> >  	struct kvm_memory_slot *ms;
> >  
> > +	if (unlikely(!slots->used_slots))
> > +		return 0;
> > +  

this should never happen, as this function is only called during
migration, and if we don't have any memory slots, then we will not try
to migrate them. 

But this is something that is triggered by userspace, so we need to
protect the kernel from rogue or broken userspace.

Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

> this looks sane and like the right fix.
> 
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> >  	cur_gfn = kvm_s390_next_dirty_cmma(slots, args->start_gfn);
> >  	ms = gfn_to_memslot(kvm, cur_gfn);
> >  	args->count = 0;
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 35bc52e187a2..b19dee4ed7d9 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -1032,6 +1032,9 @@ search_memslots(struct kvm_memslots *slots,
> > gfn_t gfn) int slot = atomic_read(&slots->lru_slot);
> >  	struct kvm_memory_slot *memslots = slots->memslots;
> >  
> > +	if (unlikely(!slots->used_slots))
> > +		return NULL;
> > +
> >  	if (gfn >= memslots[slot].base_gfn &&
> >  	    gfn < memslots[slot].base_gfn + memslots[slot].npages)
> >  		return &memslots[slot];
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 28eae681859f..f744bc603c53 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -882,6 +882,9 @@ static inline void kvm_memslot_delete(struct
> > kvm_memslots *slots, 
> >  	slots->used_slots--;
> >  
> > +	if (atomic_read(&slots->lru_slot) >= slots->used_slots)
> > +		atomic_set(&slots->lru_slot, 0);
> > +
> >  	for (i = slots->id_to_index[memslot->id]; i <
> > slots->used_slots; i++) { mslots[i] = mslots[i + 1];
> >  		slots->id_to_index[mslots[i].id] = i;
> >   

[PATCH 2/7] KVM: selftests: Fix cosmetic copy-paste error in vm_mem_region_move()

[Sean Christopherson]

Fix a copy-paste typo in a comment and error message.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/kvm/lib/kvm_util.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 0cf98ad59e32..8a3523d4434f 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -764,7 +764,7 @@ void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags)
  * Input Args:
  *   vm - Virtual Machine
  *   slot - Slot of the memory region to move
- *   flags - Starting guest physical address
+ *   new_gpa - Starting guest physical address
  *
  * Output Args: None
  *
@@ -784,7 +784,7 @@ void vm_mem_region_move(struct kvm_vm *vm, uint32_t slot, uint64_t new_gpa)
 	ret = ioctl(vm->fd, KVM_SET_USER_MEMORY_REGION, &region->region);
 
 	TEST_ASSERT(!ret, "KVM_SET_USER_MEMORY_REGION failed\n"
-		    "ret: %i errno: %i slot: %u flags: 0x%lx",
+		    "ret: %i errno: %i slot: %u new_gpa: 0x%lx",
 		    ret, errno, slot, new_gpa);
 }
 
-- 
2.24.1

[PATCH 3/7] KVM: selftests: Take vcpu pointer instead of id in vm_vcpu_rm()

[Sean Christopherson]

The sole caller of vm_vcpu_rm() already has the vcpu pointer, take it
directly instead of doing an extra lookup.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/kvm/lib/kvm_util.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 8a3523d4434f..9a783c20dd26 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -393,7 +393,7 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
  *
  * Input Args:
  *   vm - Virtual Machine
- *   vcpuid - VCPU ID
+ *   vcpu - VCPU to remove
  *
  * Output Args: None
  *
@@ -401,9 +401,8 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
  *
  * Within the VM specified by vm, removes the VCPU given by vcpuid.
  */
-static void vm_vcpu_rm(struct kvm_vm *vm, uint32_t vcpuid)
+static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
 {
-	struct vcpu *vcpu = vcpu_find(vm, vcpuid);
 	int ret;
 
 	ret = munmap(vcpu->state, sizeof(*vcpu->state));
@@ -427,7 +426,7 @@ void kvm_vm_release(struct kvm_vm *vmp)
 	int ret;
 
 	while (vmp->vcpu_head)
-		vm_vcpu_rm(vmp, vmp->vcpu_head->id);
+		vm_vcpu_rm(vmp, vmp->vcpu_head);
 
 	ret = close(vmp->fd);
 	TEST_ASSERT(ret == 0, "Close of vm fd failed,\n"
-- 
2.24.1

[PATCH 4/7] KVM: selftests: Add helpers to consolidate open coded list operations

[Sean Christopherson]

Add helpers for the KVM sefltests' variant of a linked list to replace a
variety of open coded adds, deletes and iterators.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/kvm/lib/kvm_util.c | 68 ++++++++++++----------
 1 file changed, 37 insertions(+), 31 deletions(-)

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 9a783c20dd26..d7b74f465570 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -19,6 +19,27 @@
 #define KVM_UTIL_PGS_PER_HUGEPG 512
 #define KVM_UTIL_MIN_PFN	2
 
+#define kvm_list_add(head, new)		\
+do {					\
+	if (head)			\
+		head->prev = new;	\
+	new->next = head;		\
+	head = new;			\
+} while (0)
+
+#define kvm_list_del(head, del)			\
+do {						\
+	if (del->next)				\
+		del->next->prev = del->prev;	\
+	if (del->prev)				\
+		del->prev->next = del->next;	\
+	else					\
+		head = del->next;		\
+} while (0)
+
+#define kvm_list_for_each(head, iter)		\
+	for (iter = head; iter; iter = iter->next)
+
 /* Aligns x up to the next multiple of size. Size must be a power of 2. */
 static void *align(void *x, size_t size)
 {
@@ -258,8 +279,7 @@ void kvm_vm_restart(struct kvm_vm *vmp, int perm)
 	if (vmp->has_irqchip)
 		vm_create_irqchip(vmp);
 
-	for (region = vmp->userspace_mem_region_head; region;
-		region = region->next) {
+	kvm_list_for_each(vmp->userspace_mem_region_head, region) {
 		int ret = ioctl(vmp->fd, KVM_SET_USER_MEMORY_REGION, &region->region);
 		TEST_ASSERT(ret == 0, "KVM_SET_USER_MEMORY_REGION IOCTL failed,\n"
 			    "  rc: %i errno: %i\n"
@@ -319,8 +339,7 @@ userspace_mem_region_find(struct kvm_vm *vm, uint64_t start, uint64_t end)
 {
 	struct userspace_mem_region *region;
 
-	for (region = vm->userspace_mem_region_head; region;
-		region = region->next) {
+	kvm_list_for_each(vm->userspace_mem_region_head, region) {
 		uint64_t existing_start = region->region.guest_phys_addr;
 		uint64_t existing_end = region->region.guest_phys_addr
 			+ region->region.memory_size - 1;
@@ -380,7 +399,7 @@ struct vcpu *vcpu_find(struct kvm_vm *vm, uint32_t vcpuid)
 {
 	struct vcpu *vcpup;
 
-	for (vcpup = vm->vcpu_head; vcpup; vcpup = vcpup->next) {
+	kvm_list_for_each(vm->vcpu_head, vcpup) {
 		if (vcpup->id == vcpuid)
 			return vcpup;
 	}
@@ -412,12 +431,7 @@ static void vm_vcpu_rm(struct kvm_vm *vm, struct vcpu *vcpu)
 	TEST_ASSERT(ret == 0, "Close of VCPU fd failed, rc: %i "
 		"errno: %i", ret, errno);
 
-	if (vcpu->next)
-		vcpu->next->prev = vcpu->prev;
-	if (vcpu->prev)
-		vcpu->prev->next = vcpu->next;
-	else
-		vm->vcpu_head = vcpu->next;
+	kvm_list_del(vm->vcpu_head, vcpu);
 	free(vcpu);
 }
 
@@ -452,13 +466,14 @@ void kvm_vm_free(struct kvm_vm *vmp)
 		struct userspace_mem_region *region
 			= vmp->userspace_mem_region_head;
 
+		kvm_list_del(vmp->userspace_mem_region_head, region);
+
 		region->region.memory_size = 0;
 		ret = ioctl(vmp->fd, KVM_SET_USER_MEMORY_REGION,
 			&region->region);
 		TEST_ASSERT(ret == 0, "KVM_SET_USER_MEMORY_REGION IOCTL failed, "
 			"rc: %i errno: %i", ret, errno);
 
-		vmp->userspace_mem_region_head = region->next;
 		sparsebit_free(&region->unused_phy_pages);
 		ret = munmap(region->mmap_start, region->mmap_size);
 		TEST_ASSERT(ret == 0, "munmap failed, rc: %i errno: %i",
@@ -611,8 +626,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
 			(uint64_t) region->region.memory_size);
 
 	/* Confirm no region with the requested slot already exists. */
-	for (region = vm->userspace_mem_region_head; region;
-		region = region->next) {
+	kvm_list_for_each(vm->userspace_mem_region_head, region) {
 		if (region->region.slot == slot)
 			break;
 	}
@@ -685,10 +699,7 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
 		guest_paddr, (uint64_t) region->region.memory_size);
 
 	/* Add to linked-list of memory regions. */
-	if (vm->userspace_mem_region_head)
-		vm->userspace_mem_region_head->prev = region;
-	region->next = vm->userspace_mem_region_head;
-	vm->userspace_mem_region_head = region;
+	kvm_list_add(vm->userspace_mem_region_head, region);
 }
 
 /*
@@ -711,8 +722,7 @@ memslot2region(struct kvm_vm *vm, uint32_t memslot)
 {
 	struct userspace_mem_region *region;
 
-	for (region = vm->userspace_mem_region_head; region;
-		region = region->next) {
+	kvm_list_for_each(vm->userspace_mem_region_head, region) {
 		if (region->region.slot == memslot)
 			break;
 	}
@@ -862,10 +872,7 @@ void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid)
 		"vcpu id: %u errno: %i", vcpuid, errno);
 
 	/* Add to linked-list of VCPUs. */
-	if (vm->vcpu_head)
-		vm->vcpu_head->prev = vcpu;
-	vcpu->next = vm->vcpu_head;
-	vm->vcpu_head = vcpu;
+	kvm_list_add(vm->vcpu_head, vcpu);
 }
 
 /*
@@ -1058,8 +1065,8 @@ void virt_map(struct kvm_vm *vm, uint64_t vaddr, uint64_t paddr,
 void *addr_gpa2hva(struct kvm_vm *vm, vm_paddr_t gpa)
 {
 	struct userspace_mem_region *region;
-	for (region = vm->userspace_mem_region_head; region;
-	     region = region->next) {
+
+	kvm_list_for_each(vm->userspace_mem_region_head, region) {
 		if ((gpa >= region->region.guest_phys_addr)
 			&& (gpa <= (region->region.guest_phys_addr
 				+ region->region.memory_size - 1)))
@@ -1091,8 +1098,8 @@ void *addr_gpa2hva(struct kvm_vm *vm, vm_paddr_t gpa)
 vm_paddr_t addr_hva2gpa(struct kvm_vm *vm, void *hva)
 {
 	struct userspace_mem_region *region;
-	for (region = vm->userspace_mem_region_head; region;
-	     region = region->next) {
+
+	kvm_list_for_each(vm->userspace_mem_region_head, region) {
 		if ((hva >= region->host_mem)
 			&& (hva <= (region->host_mem
 				+ region->region.memory_size - 1)))
@@ -1519,8 +1526,7 @@ void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
 	fprintf(stream, "%*sfd: %i\n", indent, "", vm->fd);
 	fprintf(stream, "%*spage_size: 0x%x\n", indent, "", vm->page_size);
 	fprintf(stream, "%*sMem Regions:\n", indent, "");
-	for (region = vm->userspace_mem_region_head; region;
-		region = region->next) {
+	kvm_list_for_each(vm->userspace_mem_region_head, region) {
 		fprintf(stream, "%*sguest_phys: 0x%lx size: 0x%lx "
 			"host_virt: %p\n", indent + 2, "",
 			(uint64_t) region->region.guest_phys_addr,
@@ -1539,7 +1545,7 @@ void vm_dump(FILE *stream, struct kvm_vm *vm, uint8_t indent)
 		virt_dump(stream, vm, indent + 4);
 	}
 	fprintf(stream, "%*sVCPUs:\n", indent, "");
-	for (vcpu = vm->vcpu_head; vcpu; vcpu = vcpu->next)
+	kvm_list_for_each(vm->vcpu_head, vcpu)
 		vcpu_dump(stream, vm, vcpu->id, indent + 2);
 }
 
-- 
2.24.1

Re: [PATCH 4/7] KVM: selftests: Add helpers to consolidate open coded list operations

[Peter Xu]

On Fri, Mar 20, 2020 at 01:55:43PM -0700, Sean Christopherson wrote:
> Add helpers for the KVM sefltests' variant of a linked list to replace a
> variety of open coded adds, deletes and iterators.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  tools/testing/selftests/kvm/lib/kvm_util.c | 68 ++++++++++++----------
>  1 file changed, 37 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 9a783c20dd26..d7b74f465570 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -19,6 +19,27 @@
>  #define KVM_UTIL_PGS_PER_HUGEPG 512
>  #define KVM_UTIL_MIN_PFN	2
>  
> +#define kvm_list_add(head, new)		\
> +do {					\
> +	if (head)			\
> +		head->prev = new;	\
> +	new->next = head;		\
> +	head = new;			\
> +} while (0)
> +
> +#define kvm_list_del(head, del)			\
> +do {						\
> +	if (del->next)				\
> +		del->next->prev = del->prev;	\
> +	if (del->prev)				\
> +		del->prev->next = del->next;	\
> +	else					\
> +		head = del->next;		\
> +} while (0)
> +
> +#define kvm_list_for_each(head, iter)		\
> +	for (iter = head; iter; iter = iter->next)
> +

I'm not sure whether we should start to use a common list, e.g.,
tools/include/linux/list.h, if we're going to rework them after all...
Even if this is preferred, maybe move to a header so kvm selftests can
use it in the future outside "vcpu" struct too and this file only?

Thanks,

-- 
Peter Xu

Re: [PATCH 4/7] KVM: selftests: Add helpers to consolidate open coded list operations

[Paolo Bonzini]

On 20/03/20 23:47, Peter Xu wrote:
> I'm not sure whether we should start to use a common list, e.g.,
> tools/include/linux/list.h, if we're going to rework them after all...
> Even if this is preferred, maybe move to a header so kvm selftests can
> use it in the future outside "vcpu" struct too and this file only?

Yes, we should.

Paolo

[PATCH 5/7] KVM: selftests: Add util to delete memory region

[Sean Christopherson]

Add a utility to delete a memory region, it will be used by x86's
set_memory_region_test.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 .../testing/selftests/kvm/include/kvm_util.h  |  1 +
 tools/testing/selftests/kvm/lib/kvm_util.c    | 60 ++++++++++++-------
 2 files changed, 40 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index a99b875f50d2..0f0e86e188c4 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -113,6 +113,7 @@ int _vcpu_ioctl(struct kvm_vm *vm, uint32_t vcpuid, unsigned long ioctl,
 void vm_ioctl(struct kvm_vm *vm, unsigned long ioctl, void *arg);
 void vm_mem_region_set_flags(struct kvm_vm *vm, uint32_t slot, uint32_t flags);
 void vm_mem_region_move(struct kvm_vm *vm, uint32_t slot, uint64_t new_gpa);
+void vm_mem_region_delete(struct kvm_vm *vm, uint32_t slot);
 void vm_vcpu_add(struct kvm_vm *vm, uint32_t vcpuid);
 vm_vaddr_t vm_vaddr_alloc(struct kvm_vm *vm, size_t sz, vm_vaddr_t vaddr_min,
 			  uint32_t data_memslot, uint32_t pgd_memslot);
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index d7b74f465570..f69fa84c9a4c 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -451,36 +451,36 @@ void kvm_vm_release(struct kvm_vm *vmp)
 		"  vmp->kvm_fd: %i rc: %i errno: %i", vmp->kvm_fd, ret, errno);
 }
 
+static void __vm_mem_region_delete(struct kvm_vm *vm,
+				   struct userspace_mem_region *region)
+{
+	int ret;
+
+	kvm_list_del(vm->userspace_mem_region_head, region);
+
+	region->region.memory_size = 0;
+	ret = ioctl(vm->fd, KVM_SET_USER_MEMORY_REGION, &region->region);
+	TEST_ASSERT(ret == 0, "KVM_SET_USER_MEMORY_REGION IOCTL failed, "
+		    "rc: %i errno: %i", ret, errno);
+
+	sparsebit_free(&region->unused_phy_pages);
+	ret = munmap(region->mmap_start, region->mmap_size);
+	TEST_ASSERT(ret == 0, "munmap failed, rc: %i errno: %i", ret, errno);
+
+	free(region);
+}
+
 /*
  * Destroys and frees the VM pointed to by vmp.
  */
 void kvm_vm_free(struct kvm_vm *vmp)
 {
-	int ret;
-
 	if (vmp == NULL)
 		return;
 
 	/* Free userspace_mem_regions. */
-	while (vmp->userspace_mem_region_head) {
-		struct userspace_mem_region *region
-			= vmp->userspace_mem_region_head;
-
-		kvm_list_del(vmp->userspace_mem_region_head, region);
-
-		region->region.memory_size = 0;
-		ret = ioctl(vmp->fd, KVM_SET_USER_MEMORY_REGION,
-			&region->region);
-		TEST_ASSERT(ret == 0, "KVM_SET_USER_MEMORY_REGION IOCTL failed, "
-			"rc: %i errno: %i", ret, errno);
-
-		sparsebit_free(&region->unused_phy_pages);
-		ret = munmap(region->mmap_start, region->mmap_size);
-		TEST_ASSERT(ret == 0, "munmap failed, rc: %i errno: %i",
-			    ret, errno);
-
-		free(region);
-	}
+	while (vmp->userspace_mem_region_head)
+		__vm_mem_region_delete(vmp, vmp->userspace_mem_region_head);
 
 	/* Free sparsebit arrays. */
 	sparsebit_free(&vmp->vpages_valid);
@@ -797,6 +797,24 @@ void vm_mem_region_move(struct kvm_vm *vm, uint32_t slot, uint64_t new_gpa)
 		    ret, errno, slot, new_gpa);
 }
 
+/*
+ * VM Memory Region Delete
+ *
+ * Input Args:
+ *   vm - Virtual Machine
+ *   slot - Slot of the memory region to delete
+ *
+ * Output Args: None
+ *
+ * Return: None
+ *
+ * Delete a memory region.
+ */
+void vm_mem_region_delete(struct kvm_vm *vm, uint32_t slot)
+{
+	__vm_mem_region_delete(vm, memslot2region(vm, slot));
+}
+
 /*
  * VCPU mmap Size
  *
-- 
2.24.1

[PATCH 6/7] KVM: selftests: Expose the primary memslot number to tests

[Sean Christopherson]

Add a define for the primary memslot number so that tests can manipulate
the memslot, e.g. to delete it.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 tools/testing/selftests/kvm/include/kvm_util.h | 2 ++
 tools/testing/selftests/kvm/lib/kvm_util.c     | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
index 0f0e86e188c4..43b5feb546c6 100644
--- a/tools/testing/selftests/kvm/include/kvm_util.h
+++ b/tools/testing/selftests/kvm/include/kvm_util.h
@@ -60,6 +60,8 @@ enum vm_mem_backing_src_type {
 	VM_MEM_SRC_ANONYMOUS_HUGETLB,
 };
 
+#define VM_PRIMARY_MEM_SLOT	0
+
 int kvm_check_cap(long cap);
 int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
 
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index f69fa84c9a4c..6a1af0455e44 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -247,8 +247,8 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm)
 	/* Allocate and setup memory for guest. */
 	vm->vpages_mapped = sparsebit_alloc();
 	if (phy_pages != 0)
-		vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
-					    0, 0, phy_pages, 0);
+		vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0,
+					    VM_PRIMARY_MEM_SLOT, phy_pages, 0);
 
 	return vm;
 }
-- 
2.24.1

Re: [PATCH 6/7] KVM: selftests: Expose the primary memslot number to tests

[Peter Xu]

On Fri, Mar 20, 2020 at 01:55:45PM -0700, Sean Christopherson wrote:
> Add a define for the primary memslot number so that tests can manipulate
> the memslot, e.g. to delete it.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  tools/testing/selftests/kvm/include/kvm_util.h | 2 ++
>  tools/testing/selftests/kvm/lib/kvm_util.c     | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> index 0f0e86e188c4..43b5feb546c6 100644
> --- a/tools/testing/selftests/kvm/include/kvm_util.h
> +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> @@ -60,6 +60,8 @@ enum vm_mem_backing_src_type {
>  	VM_MEM_SRC_ANONYMOUS_HUGETLB,
>  };
>  
> +#define VM_PRIMARY_MEM_SLOT	0
> +
>  int kvm_check_cap(long cap);
>  int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
>  
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index f69fa84c9a4c..6a1af0455e44 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -247,8 +247,8 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm)
>  	/* Allocate and setup memory for guest. */
>  	vm->vpages_mapped = sparsebit_alloc();
>  	if (phy_pages != 0)
> -		vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
> -					    0, 0, phy_pages, 0);
> +		vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0,
> +					    VM_PRIMARY_MEM_SLOT, phy_pages, 0);

IIUC VM_PRIMARY_MEM_SLOT should be used more than here... E.g., to all
the places that allocate page tables in virt_map() as the last param?
I didn't check other places.

Maybe it's simpler to drop this patch for now and use 0 directly as
before for now, after all in the last patch the comment is good enough
for me to understand slot 0 is the default slot.

Thanks,

>  
>  	return vm;
>  }
> -- 
> 2.24.1
> 

-- 
Peter Xu

Re: [PATCH 6/7] KVM: selftests: Expose the primary memslot number to tests

[Sean Christopherson]

On Mon, Mar 23, 2020 at 03:12:02PM -0400, Peter Xu wrote:
> On Fri, Mar 20, 2020 at 01:55:45PM -0700, Sean Christopherson wrote:
> > Add a define for the primary memslot number so that tests can manipulate
> > the memslot, e.g. to delete it.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> > ---
> >  tools/testing/selftests/kvm/include/kvm_util.h | 2 ++
> >  tools/testing/selftests/kvm/lib/kvm_util.c     | 4 ++--
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index 0f0e86e188c4..43b5feb546c6 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -60,6 +60,8 @@ enum vm_mem_backing_src_type {
> >  	VM_MEM_SRC_ANONYMOUS_HUGETLB,
> >  };
> >  
> > +#define VM_PRIMARY_MEM_SLOT	0
> > +
> >  int kvm_check_cap(long cap);
> >  int vm_enable_cap(struct kvm_vm *vm, struct kvm_enable_cap *cap);
> >  
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index f69fa84c9a4c..6a1af0455e44 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -247,8 +247,8 @@ struct kvm_vm *_vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm)
> >  	/* Allocate and setup memory for guest. */
> >  	vm->vpages_mapped = sparsebit_alloc();
> >  	if (phy_pages != 0)
> > -		vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
> > -					    0, 0, phy_pages, 0);
> > +		vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, 0,
> > +					    VM_PRIMARY_MEM_SLOT, phy_pages, 0);
> 
> IIUC VM_PRIMARY_MEM_SLOT should be used more than here... E.g., to all
> the places that allocate page tables in virt_map() as the last param?
> I didn't check other places.

Ouch, yeah, it bleeds into vm_vaddr_alloc() as well.
 
> Maybe it's simpler to drop this patch for now and use 0 directly as
> before for now, after all in the last patch the comment is good enough
> for me to understand slot 0 is the default slot.

Ya, I'll drop this and hardcode '0', it's a rather absurd amount of call
sites to change.

[PATCH 7/7] KVM: selftests: Add "delete" testcase to set_memory_region_test

[Sean Christopherson]

Add coverate for running a guest with no memslots, and for deleting
memslots while the guest is running.  Enhance the test to use, and
expect, a unique value for MMIO reads, e.g. to verify each stage of
the test.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 .../kvm/x86_64/set_memory_region_test.c       | 122 ++++++++++++++++--
 1 file changed, 108 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/set_memory_region_test.c b/tools/testing/selftests/kvm/x86_64/set_memory_region_test.c
index c6691cff4e19..44aed8ac932b 100644
--- a/tools/testing/selftests/kvm/x86_64/set_memory_region_test.c
+++ b/tools/testing/selftests/kvm/x86_64/set_memory_region_test.c
@@ -26,42 +26,109 @@
 #define MEM_REGION_SIZE		0x200000
 #define MEM_REGION_SLOT		10
 
-static void guest_code(void)
+static const uint64_t MMIO_VAL = 0xbeefull;
+
+extern const uint64_t final_rip_start;
+extern const uint64_t final_rip_end;
+
+static inline uint64_t guest_spin_on_val(uint64_t spin_val)
 {
 	uint64_t val;
 
 	do {
 		val = READ_ONCE(*((uint64_t *)MEM_REGION_GPA));
-	} while (!val);
+	} while (val == spin_val);
+	return val;
+}
 
-	if (val != 1)
-		ucall(UCALL_ABORT, 1, val);
+static void guest_code(void)
+{
+	uint64_t val;
 
-	GUEST_DONE();
+	/*
+	 * Spin until the memory region is moved to a misaligned address.  This
+	 * may or may not trigger MMIO, as the window where the memslot is
+	 * invalid is quite small.
+	 */
+	val = guest_spin_on_val(0);
+	GUEST_ASSERT(val == 1 || val == MMIO_VAL);
+
+	/* Spin until the memory region is realigned. */
+	GUEST_ASSERT(guest_spin_on_val(MMIO_VAL) == 1);
+
+	/* Spin until the memory region is deleted. */
+	GUEST_ASSERT(guest_spin_on_val(1) == MMIO_VAL);
+
+	/* Spin until the memory region is recreated. */
+	GUEST_ASSERT(guest_spin_on_val(MMIO_VAL) == 0);
+
+	/* Spin until the memory region is deleted. */
+	GUEST_ASSERT(guest_spin_on_val(0) == MMIO_VAL);
+
+	asm("1:\n\t"
+	    ".pushsection .rodata\n\t"
+	    ".global final_rip_start\n\t"
+	    "final_rip_start: .quad 1b\n\t"
+	    ".popsection");
+
+	/* Spin indefinitely (until the code memslot is deleted). */
+	guest_spin_on_val(MMIO_VAL);
+
+	asm("1:\n\t"
+	    ".pushsection .rodata\n\t"
+	    ".global final_rip_end\n\t"
+	    "final_rip_end: .quad 1b\n\t"
+	    ".popsection");
+
+	GUEST_ASSERT(0);
 }
 
 static void *vcpu_worker(void *data)
 {
 	struct kvm_vm *vm = data;
+	struct kvm_regs regs;
 	struct kvm_run *run;
 	struct ucall uc;
-	uint64_t cmd;
 
 	/*
 	 * Loop until the guest is done.  Re-enter the guest on all MMIO exits,
-	 * which will occur if the guest attempts to access a memslot while it
-	 * is being moved.
+	 * which will occur if the guest attempts to access a memslot after it
+	 * has been deleted or while it is being moved .
 	 */
 	run = vcpu_state(vm, VCPU_ID);
-	do {
+
+	memcpy(run->mmio.data, &MMIO_VAL, 8);
+	while (1) {
 		vcpu_run(vm, VCPU_ID);
-	} while (run->exit_reason == KVM_EXIT_MMIO);
+		if (run->exit_reason != KVM_EXIT_MMIO)
+			break;
 
-	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+		TEST_ASSERT(!run->mmio.is_write, "Unexpected exit mmio write");
+		TEST_ASSERT(run->mmio.len == 8,
+			    "Unexpected exit mmio size = %u", run->mmio.len);
+
+		TEST_ASSERT(run->mmio.phys_addr == MEM_REGION_GPA,
+			    "Unexpected exit mmio address = 0x%llx",
+			    run->mmio.phys_addr);
+	}
+
+	if (run->exit_reason == KVM_EXIT_IO) {
+		(void)get_ucall(vm, VCPU_ID, &uc);
+		TEST_FAIL("%s at %s:%ld",
+			  (const char *)uc.args[0], __FILE__, uc.args[1]);
+	}
+
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_SHUTDOWN ||
+		    run->exit_reason == KVM_INTERNAL_ERROR_EMULATION,
 		    "Unexpected exit reason = %d", run->exit_reason);
 
-	cmd = get_ucall(vm, VCPU_ID, &uc);
-	TEST_ASSERT(cmd == UCALL_DONE, "Unexpected val in guest = %lu", uc.args[0]);
+	vcpu_regs_get(vm, VCPU_ID, &regs);
+
+	TEST_ASSERT(regs.rip >= final_rip_start &&
+		    regs.rip < final_rip_end,
+		    "Bad rip, expected 0x%lx - 0x%lx, got 0x%llx\n",
+		    final_rip_start, final_rip_end, regs.rip);
+
 	return NULL;
 }
 
@@ -72,6 +139,13 @@ static void test_move_memory_region(void)
 	uint64_t *hva;
 	uint64_t gpa;
 
+	vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
+	vm_vcpu_add(vm, VCPU_ID);
+	/* Fails with ENOSPC because the MMU can't create pages (no slots). */
+	TEST_ASSERT(_vcpu_run(vm, VCPU_ID) == -1 && errno == ENOSPC,
+		    "Unexpected error code = %d", errno);
+	kvm_vm_free(vm);
+
 	vm = vm_create_default(VCPU_ID, 0, guest_code);
 
 	vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
@@ -105,7 +179,6 @@ static void test_move_memory_region(void)
 	 */
 	vm_mem_region_move(vm, MEM_REGION_SLOT, MEM_REGION_GPA - 4096);
 	WRITE_ONCE(*hva, 2);
-
 	usleep(100000);
 
 	/*
@@ -116,6 +189,27 @@ static void test_move_memory_region(void)
 
 	/* Restore the original base, the guest should see "1". */
 	vm_mem_region_move(vm, MEM_REGION_SLOT, MEM_REGION_GPA);
+	usleep(100000);
+
+	/* Delete the memory region, the guest should not die. */
+	vm_mem_region_delete(vm, MEM_REGION_SLOT);
+	usleep(100000);
+
+	/* Recreate the memory region.  The guest should see "0". */
+	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS_THP,
+				    MEM_REGION_GPA, MEM_REGION_SLOT,
+				    MEM_REGION_SIZE / getpagesize(), 0);
+	usleep(100000);
+
+	/* Delete the region again so that there's only one memslot left. */
+	vm_mem_region_delete(vm, MEM_REGION_SLOT);
+	usleep(100000);
+
+	/*
+	 * Delete the primary memslot.  This should cause an emulation error or
+	 * shutdown due to the page tables getting nuked.
+	 */
+	vm_mem_region_delete(vm, VM_PRIMARY_MEM_SLOT);
 
 	pthread_join(vcpu_thread, NULL);
 
-- 
2.24.1

Re: [PATCH 7/7] KVM: selftests: Add "delete" testcase to set_memory_region_test

[Peter Xu]

On Fri, Mar 20, 2020 at 01:55:46PM -0700, Sean Christopherson wrote:
> Add coverate for running a guest with no memslots, and for deleting
> memslots while the guest is running.  Enhance the test to use, and
> expect, a unique value for MMIO reads, e.g. to verify each stage of
> the test.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  .../kvm/x86_64/set_memory_region_test.c       | 122 ++++++++++++++++--
>  1 file changed, 108 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/set_memory_region_test.c b/tools/testing/selftests/kvm/x86_64/set_memory_region_test.c
> index c6691cff4e19..44aed8ac932b 100644
> --- a/tools/testing/selftests/kvm/x86_64/set_memory_region_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/set_memory_region_test.c
> @@ -26,42 +26,109 @@
>  #define MEM_REGION_SIZE		0x200000
>  #define MEM_REGION_SLOT		10
>  
> -static void guest_code(void)
> +static const uint64_t MMIO_VAL = 0xbeefull;
> +
> +extern const uint64_t final_rip_start;
> +extern const uint64_t final_rip_end;
> +
> +static inline uint64_t guest_spin_on_val(uint64_t spin_val)
>  {
>  	uint64_t val;
>  
>  	do {
>  		val = READ_ONCE(*((uint64_t *)MEM_REGION_GPA));
> -	} while (!val);
> +	} while (val == spin_val);
> +	return val;
> +}
>  
> -	if (val != 1)
> -		ucall(UCALL_ABORT, 1, val);
> +static void guest_code(void)
> +{
> +	uint64_t val;
>  
> -	GUEST_DONE();
> +	/*
> +	 * Spin until the memory region is moved to a misaligned address.  This
> +	 * may or may not trigger MMIO, as the window where the memslot is
> +	 * invalid is quite small.
> +	 */
> +	val = guest_spin_on_val(0);
> +	GUEST_ASSERT(val == 1 || val == MMIO_VAL);
> +
> +	/* Spin until the memory region is realigned. */
> +	GUEST_ASSERT(guest_spin_on_val(MMIO_VAL) == 1);

IIUC ideally we should do GUEST_SYNC() after each GUEST_ASSERT() to
make sure the two threads are in sync.  Otherwise e.g. there's no
guarantee that the main thread won't run too fast to quickly remove
the memslot and re-add it back before the guest_spin_on_val() starts
above, then the assert could trigger when it reads the value as zero.

> +
> +	/* Spin until the memory region is deleted. */
> +	GUEST_ASSERT(guest_spin_on_val(1) == MMIO_VAL);
> +
> +	/* Spin until the memory region is recreated. */
> +	GUEST_ASSERT(guest_spin_on_val(MMIO_VAL) == 0);
> +
> +	/* Spin until the memory region is deleted. */
> +	GUEST_ASSERT(guest_spin_on_val(0) == MMIO_VAL);
> +
> +	asm("1:\n\t"
> +	    ".pushsection .rodata\n\t"
> +	    ".global final_rip_start\n\t"
> +	    "final_rip_start: .quad 1b\n\t"
> +	    ".popsection");
> +
> +	/* Spin indefinitely (until the code memslot is deleted). */
> +	guest_spin_on_val(MMIO_VAL);
> +
> +	asm("1:\n\t"
> +	    ".pushsection .rodata\n\t"
> +	    ".global final_rip_end\n\t"
> +	    "final_rip_end: .quad 1b\n\t"
> +	    ".popsection");
> +
> +	GUEST_ASSERT(0);
>  }
>  
>  static void *vcpu_worker(void *data)
>  {
>  	struct kvm_vm *vm = data;
> +	struct kvm_regs regs;
>  	struct kvm_run *run;
>  	struct ucall uc;
> -	uint64_t cmd;
>  
>  	/*
>  	 * Loop until the guest is done.  Re-enter the guest on all MMIO exits,
> -	 * which will occur if the guest attempts to access a memslot while it
> -	 * is being moved.
> +	 * which will occur if the guest attempts to access a memslot after it
> +	 * has been deleted or while it is being moved .
>  	 */
>  	run = vcpu_state(vm, VCPU_ID);
> -	do {
> +
> +	memcpy(run->mmio.data, &MMIO_VAL, 8);
> +	while (1) {
>  		vcpu_run(vm, VCPU_ID);
> -	} while (run->exit_reason == KVM_EXIT_MMIO);
> +		if (run->exit_reason != KVM_EXIT_MMIO)
> +			break;
>  
> -	TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
> +		TEST_ASSERT(!run->mmio.is_write, "Unexpected exit mmio write");
> +		TEST_ASSERT(run->mmio.len == 8,
> +			    "Unexpected exit mmio size = %u", run->mmio.len);
> +
> +		TEST_ASSERT(run->mmio.phys_addr == MEM_REGION_GPA,
> +			    "Unexpected exit mmio address = 0x%llx",
> +			    run->mmio.phys_addr);
> +	}
> +
> +	if (run->exit_reason == KVM_EXIT_IO) {
> +		(void)get_ucall(vm, VCPU_ID, &uc);
> +		TEST_FAIL("%s at %s:%ld",
> +			  (const char *)uc.args[0], __FILE__, uc.args[1]);
> +	}
> +
> +	TEST_ASSERT(run->exit_reason == KVM_EXIT_SHUTDOWN ||
> +		    run->exit_reason == KVM_INTERNAL_ERROR_EMULATION,
>  		    "Unexpected exit reason = %d", run->exit_reason);
>  
> -	cmd = get_ucall(vm, VCPU_ID, &uc);
> -	TEST_ASSERT(cmd == UCALL_DONE, "Unexpected val in guest = %lu", uc.args[0]);
> +	vcpu_regs_get(vm, VCPU_ID, &regs);
> +
> +	TEST_ASSERT(regs.rip >= final_rip_start &&
> +		    regs.rip < final_rip_end,
> +		    "Bad rip, expected 0x%lx - 0x%lx, got 0x%llx\n",
> +		    final_rip_start, final_rip_end, regs.rip);
> +
>  	return NULL;
>  }
>  
> @@ -72,6 +139,13 @@ static void test_move_memory_region(void)
>  	uint64_t *hva;
>  	uint64_t gpa;
>  
> +	vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
> +	vm_vcpu_add(vm, VCPU_ID);
> +	/* Fails with ENOSPC because the MMU can't create pages (no slots). */
> +	TEST_ASSERT(_vcpu_run(vm, VCPU_ID) == -1 && errno == ENOSPC,
> +		    "Unexpected error code = %d", errno);
> +	kvm_vm_free(vm);
> +
>  	vm = vm_create_default(VCPU_ID, 0, guest_code);
>  
>  	vcpu_set_cpuid(vm, VCPU_ID, kvm_get_supported_cpuid());
> @@ -105,7 +179,6 @@ static void test_move_memory_region(void)
>  	 */
>  	vm_mem_region_move(vm, MEM_REGION_SLOT, MEM_REGION_GPA - 4096);
>  	WRITE_ONCE(*hva, 2);
> -
>  	usleep(100000);
>  
>  	/*
> @@ -116,6 +189,27 @@ static void test_move_memory_region(void)
>  
>  	/* Restore the original base, the guest should see "1". */
>  	vm_mem_region_move(vm, MEM_REGION_SLOT, MEM_REGION_GPA);
> +	usleep(100000);
> +
> +	/* Delete the memory region, the guest should not die. */
> +	vm_mem_region_delete(vm, MEM_REGION_SLOT);
> +	usleep(100000);
> +
> +	/* Recreate the memory region.  The guest should see "0". */
> +	vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS_THP,
> +				    MEM_REGION_GPA, MEM_REGION_SLOT,
> +				    MEM_REGION_SIZE / getpagesize(), 0);
> +	usleep(100000);
> +
> +	/* Delete the region again so that there's only one memslot left. */
> +	vm_mem_region_delete(vm, MEM_REGION_SLOT);
> +	usleep(100000);
> +
> +	/*
> +	 * Delete the primary memslot.  This should cause an emulation error or
> +	 * shutdown due to the page tables getting nuked.
> +	 */
> +	vm_mem_region_delete(vm, VM_PRIMARY_MEM_SLOT);
>  
>  	pthread_join(vcpu_thread, NULL);
>  
> -- 
> 2.24.1
> 

-- 
Peter Xu

Re: [PATCH 7/7] KVM: selftests: Add "delete" testcase to set_memory_region_test

[Sean Christopherson]

On Mon, Mar 23, 2020 at 03:06:36PM -0400, Peter Xu wrote:
> On Fri, Mar 20, 2020 at 01:55:46PM -0700, Sean Christopherson wrote:
> > +	/*
> > +	 * Spin until the memory region is moved to a misaligned address.  This
> > +	 * may or may not trigger MMIO, as the window where the memslot is
> > +	 * invalid is quite small.
> > +	 */
> > +	val = guest_spin_on_val(0);
> > +	GUEST_ASSERT(val == 1 || val == MMIO_VAL);
> > +
> > +	/* Spin until the memory region is realigned. */
> > +	GUEST_ASSERT(guest_spin_on_val(MMIO_VAL) == 1);
> 
> IIUC ideally we should do GUEST_SYNC() after each GUEST_ASSERT() to
> make sure the two threads are in sync.  Otherwise e.g. there's no
> guarantee that the main thread won't run too fast to quickly remove
> the memslot and re-add it back before the guest_spin_on_val() starts
> above, then the assert could trigger when it reads the value as zero.

Hrm, I was thinking ucall wasn't available across pthreads, but it's just
dumped into a global variable.  I'll rework this to replace the udelay()
hacks with proper synchronization.

Re: [PATCH 7/7] KVM: selftests: Add "delete" testcase to set_memory_region_test

[Peter Xu]

On Mon, Mar 23, 2020 at 02:43:18PM -0700, Sean Christopherson wrote:
> On Mon, Mar 23, 2020 at 03:06:36PM -0400, Peter Xu wrote:
> > On Fri, Mar 20, 2020 at 01:55:46PM -0700, Sean Christopherson wrote:
> > > +	/*
> > > +	 * Spin until the memory region is moved to a misaligned address.  This
> > > +	 * may or may not trigger MMIO, as the window where the memslot is
> > > +	 * invalid is quite small.
> > > +	 */
> > > +	val = guest_spin_on_val(0);
> > > +	GUEST_ASSERT(val == 1 || val == MMIO_VAL);
> > > +
> > > +	/* Spin until the memory region is realigned. */
> > > +	GUEST_ASSERT(guest_spin_on_val(MMIO_VAL) == 1);
> > 
> > IIUC ideally we should do GUEST_SYNC() after each GUEST_ASSERT() to
> > make sure the two threads are in sync.  Otherwise e.g. there's no
> > guarantee that the main thread won't run too fast to quickly remove
> > the memslot and re-add it back before the guest_spin_on_val() starts
> > above, then the assert could trigger when it reads the value as zero.
> 
> Hrm, I was thinking ucall wasn't available across pthreads, but it's just
> dumped into a global variable.  I'll rework this to replace the udelay()
> hacks with proper synchronization.

I think ucall should work for pthread (shared address space of either
kvm_run or guest memories), however my thought was even simpler than
that, something like:

  - in guest code: do GUEST_SYNC after each GUEST_ASSERT
  - introduce a global_sem
  - in vcpu thread: when receive GUEST_SYNC, do "sem_post(&global_sem)"
  - in main thread: replace all usleep() with "sem_wait(&global_sem)"

-- 
Peter Xu

Re: [PATCH 0/7] KVM: Fix memslot use-after-free bug

[Paolo Bonzini]

On 20/03/20 21:55, Sean Christopherson wrote:
> Fix a bug introduced by dynamic memslot allocation where the LRU slot can
> become invalid and lead to a out-of-bounds/use-after-free scenario.
> 
> The patch is different that what Qian has already tested, but I was able
> to reproduce the bad behavior by enhancing the set memory region selftest,
> i.e. I'm relatively confident the bug is fixed.
> 
> Patches 2-6 are a variety of selftest cleanup, with the aforementioned
> set memory region enhancement coming in patch 7.
> 
> Note, I couldn't get the selftest to fail outright or with KASAN, but was
> able to hit a WARN_ON an invalid slot 100% of the time (without the fix,
> obviously).
> 
> Regarding s390, I played around a bit with merging gfn_to_memslot_approx()
> into search_memslots().  Code wise it's trivial since they're basically
> identical, but doing so increases the code footprint of search_memslots()
> on x86 by 30 bytes, so I ended up abandoning the effort.
> 
> Sean Christopherson (7):
>   KVM: Fix out of range accesses to memslots
>   KVM: selftests: Fix cosmetic copy-paste error in vm_mem_region_move()
>   KVM: selftests: Take vcpu pointer instead of id in vm_vcpu_rm()
>   KVM: selftests: Add helpers to consolidate open coded list operations
>   KVM: selftests: Add util to delete memory region
>   KVM: selftests: Expose the primary memslot number to tests
>   KVM: selftests: Add "delete" testcase to set_memory_region_test
> 
>  arch/s390/kvm/kvm-s390.c                      |   3 +
>  include/linux/kvm_host.h                      |   3 +
>  .../testing/selftests/kvm/include/kvm_util.h  |   3 +
>  tools/testing/selftests/kvm/lib/kvm_util.c    | 139 ++++++++++--------
>  .../kvm/x86_64/set_memory_region_test.c       | 122 +++++++++++++--
>  virt/kvm/kvm_main.c                           |   3 +
>  6 files changed, 201 insertions(+), 72 deletions(-)
> 

Queued patches 1-3, thanks.

Paolo