Re: [PATCH] always assign userspace_addr

[Anthony Liguori <anthony@codemonkey.ws>]

Glauber Costa wrote:
> On Wed, Nov 19, 2008 at 09:55:10AM -0600, Anthony Liguori wrote:
>   
>> 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.
>>     
>
> Oh yeah, it does get freed.
>
> The delete path ends up in a kvm_free_physmem_slot, which will effectively
> vfree() the rmap structure. In fact, my userspace use case worked totally
> properly when I deleted the slot prior to re-registering it.
>   

That's not how I read the code.  I see:

>
> static void kvm_free_physmem_slot(struct kvm_memory_slot *free,
>                   struct kvm_memory_slot *dont)
> {
>     if (!dont || free->rmap != dont->rmap)
>         vfree(free->rmap);

And it's called as kvm_free_physmem_slot(&old, &new);

new is assigned to old to start out with so old.rmap will equal new.rmap.

> The problem here is when there is an already existant slot, and we are
> trying to change some information about it. The problem you are concerned
> basically does not exist, because it would raise only if we are changing
> the slot size. The code says:
>   

If a memory slot exists, the current code always deletes it and creates 
a new one so this problem shouldn't exist.  See

>
>             /* unregister whole slot */
>             memcpy(&slot, mem, sizeof(slot));
>             mem->memory_size = 0;
>             kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, mem);

But the problem still exists even with this code.  I checked.

So if you have something working without modifying the kernel, can you 
post it?

Regards,

Anthony Liguori