linux-kernel - Re: [PATCH] always assign userspace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <492436DE.2080906@codemonkey.ws>
Date:	Wed, 19 Nov 2008 09:55:10 -0600
From:	Anthony Liguori <anthony@...emonkey.ws>
To:	Glauber Costa <glommer@...hat.com>
CC:	linux-kernel@...r.kernel.org, kvm@...r.kernel.org, avi@...hat.com
Subject: Re: [PATCH] always assign userspace_addr

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@...hat.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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/