Re: [PATCH] always assign userspace_addr

[Anthony Liguori <anthony@codemonkey.ws>]

Glauber Costa wrote:
> Currently, kvm only sets new.userspace_addr in slots
> that were just allocated. This is not the intended behaviour,
> and actually breaks when we try to use the slots to implement
> aliases, for example.
>
> Cirrus VGA aliases maps and address to a userspace address, and
> then keep mapping this same address to different locations
> until the whole screen is filled.
>
> The solution is to assign new.userspace_addr no matter what,
> so we can be sure that whenever the guest changes this field,
> it sees the change being reflected in the code.
>
> Signed-off-by: Glauber Costa <glommer@redhat.com>
>   

I think this is masking a much bigger problem.


> ---
>  virt/kvm/kvm_main.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a87f45e..fc3abf0 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -762,15 +762,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  		memset(new.rmap, 0, npages * sizeof(*new.rmap));
>  
>  		new.user_alloc = user_alloc;
> -		/*
> -		 * hva_to_rmmap() serialzies with the mmu_lock and to be
> -		 * safe it has to ignore memslots with !user_alloc &&
> -		 * !userspace_addr.
> -		 */
> -		if (user_alloc)
> -			new.userspace_addr = mem->userspace_addr;
> -		else
> -			new.userspace_addr = 0;
>   

This is guarded in:

>     if (npages && !new.rmap) {

In this case, npages > 0 but !new.rmap is already allocated.  But this 
is a new slot?  The problem is that when we delete the slot, the rmap 
never gets freed.  This means that if we delete a slot, then create a 
new slot which happens to be a different size, we use the old rmap and 
potentially overrun that buffer.

So I think we need a fix that properly frees the rmap when the slot is 
destroyed.

Regards,

Anthony Liguori