linux-kernel - Re: [PATCH RFC] kvm: write protect memory after slot swap

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: <4CC548C1.3060009@siemens.com>
Date:	Mon, 25 Oct 2010 11:07:13 +0200
From:	Jan Kiszka <jan.kiszka@...mens.com>
To:	"Michael S. Tsirkin" <mst@...hat.com>
CC:	Avi Kivity <avi@...hat.com>, Marcelo Tosatti <mtosatti@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"x86@...nel.org" <x86@...nel.org>, Gleb Natapov <gleb@...hat.com>,
	Sheng Yang <sheng@...ux.intel.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] kvm: write protect memory after slot swap

Am 25.10.2010 03:21, Michael S. Tsirkin wrote:
> I have observed the following bug trigger:
> 
> 1. userspace calls GET_DIRTY_LOG
> 2. kvm_mmu_slot_remove_write_access is called and makes a page ro
> 3. page fault happens and makes the page writeable
>    fault is logged in the bitmap appropriately
> 4. kvm_vm_ioctl_get_dirty_log swaps slot pointers
> 
> a lot of time passes
> 
> 5. guest writes into the page
> 6. userspace calls GET_DIRTY_LOG
> 
> At point (5), bitmap is clean and page is writeable,
> thus, guest modification of memory is not logged
> and GET_DIRTY_LOG returns an empty bitmap.
> 

Cool, seems to be the key to the corruptions I've seen. Applying your
patch make them disappear.

> The rule is that all pages are either dirty in the current bitmap,
> or write-protected, which is violated here.
> 
> It seems that just moving kvm_mmu_slot_remove_write_access down
> to after the slot pointer swap should fix this bug.
> 
> Warning: completely untested.
> Please comment.
> Note: fix will be needed for -stable etc.

Assuming that a page cannot be write-enabled without having a dirty
entry in the old bitmap and due to the fact that user space won't get
hold of that old bitmap to read out the page before we reset write
access again, your patch should actually be safe.

If no one else sees some remaining race, let's get this applied upstream
ASAP and pushed down to the stable trees.

Thanks,
Jan

> 
> Signed-off-by: Michael S. Tsirkin <mst@...hat.com>
> ---
>  arch/x86/kvm/x86.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3a09c62..4ca1d7f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2912,10 +2912,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>  		struct kvm_memslots *slots, *old_slots;
>  		unsigned long *dirty_bitmap;
>  
> -		spin_lock(&kvm->mmu_lock);
> -		kvm_mmu_slot_remove_write_access(kvm, log->slot);
> -		spin_unlock(&kvm->mmu_lock);
> -
>  		r = -ENOMEM;
>  		dirty_bitmap = vmalloc(n);
>  		if (!dirty_bitmap)
> @@ -2937,6 +2933,10 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>  		dirty_bitmap = old_slots->memslots[log->slot].dirty_bitmap;
>  		kfree(old_slots);
>  
> +		spin_lock(&kvm->mmu_lock);
> +		kvm_mmu_slot_remove_write_access(kvm, log->slot);
> +		spin_unlock(&kvm->mmu_lock);
> +
>  		r = -EFAULT;
>  		if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n)) {
>  			vfree(dirty_bitmap);

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
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/