Re: [PATCH] KVM: mmu: Fix overlap with private memslots

[David Hildenbrand]

On 27.03.2017 08:23, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> 
> Reported by syzkaller:
> 
>     pte_list_remove: ffff9714eb1f8078 0->BUG
>     ------------[ cut here ]------------
>     kernel BUG at arch/x86/kvm/mmu.c:1157!
>     invalid opcode: 0000 [#1] SMP
>     RIP: 0010:pte_list_remove+0x11b/0x120 [kvm]
>     Call Trace:
>      drop_spte+0x83/0xb0 [kvm]
>      mmu_page_zap_pte+0xcc/0xe0 [kvm]
>      kvm_mmu_prepare_zap_page+0x81/0x4a0 [kvm]
>      kvm_mmu_invalidate_zap_all_pages+0x159/0x220 [kvm]
>      kvm_arch_flush_shadow_all+0xe/0x10 [kvm]
>      kvm_mmu_notifier_release+0x6c/0xa0 [kvm]
>      ? kvm_mmu_notifier_release+0x5/0xa0 [kvm]
>      __mmu_notifier_release+0x79/0x110
>      ? __mmu_notifier_release+0x5/0x110
>      exit_mmap+0x15a/0x170
>      ? do_exit+0x281/0xcb0
>      mmput+0x66/0x160
>      do_exit+0x2c9/0xcb0
>      ? __context_tracking_exit.part.5+0x4a/0x150
>      do_group_exit+0x50/0xd0
>      SyS_exit_group+0x14/0x20
>      do_syscall_64+0x73/0x1f0
>      entry_SYSCALL64_slow_path+0x25/0x25
> 
> The reason is that when creates new memslot, there is no guarantee for new 
> memslot not overlap with private memslots. This can be triggered by the 
> following program:
> 	
> #include <fcntl.h>
> #include <pthread.h>
> #include <setjmp.h>
> #include <signal.h>
> #include <stddef.h>
> #include <stdint.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/ioctl.h>
> #include <sys/stat.h>
> #include <sys/syscall.h>
> #include <sys/types.h>
> #include <unistd.h>
> #include <linux/kvm.h>
> 
> long r[16];
> 
> int main()
> {
> 	void *p = valloc(0x4000);
> 
> 	r[2] = open("/dev/kvm", 0);
> 	r[3] = ioctl(r[2], KVM_CREATE_VM, 0x0ul);
> 
> 	uint64_t addr = 0xf000;
> 	ioctl(r[3], KVM_SET_IDENTITY_MAP_ADDR, &addr);
> 	r[6] = ioctl(r[3], KVM_CREATE_VCPU, 0x0ul);
> 	ioctl(r[3], KVM_SET_TSS_ADDR, 0x0ul);
> 	ioctl(r[6], KVM_RUN, 0);
> 	ioctl(r[6], KVM_RUN, 0);
> 
> 	struct kvm_userspace_memory_region mr = {
> 		.slot = 0,
> 		.flags = KVM_MEM_LOG_DIRTY_PAGES,
> 		.guest_phys_addr = 0xf000,
> 		.memory_size = 0x4000,
> 		.userspace_addr = (uintptr_t) p
> 	};
> 	ioctl(r[3], KVM_SET_USER_MEMORY_REGION, &mr);
> 	return 0;
> }
> 
> This bug is caused by 'commit 5419369ed6bd ("KVM: Fix user memslot overlap 
> check")' which removes the check to avoid to add new memslot who overlaps 
> with private memslots. This patch fixes it by not add new memslot if it 
> is also overlap with private memslots.
> 
> Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v3.10+   
> Fixes: 5419369ed (KVM: Fix user memslot overlap check)
> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> ---
>  virt/kvm/kvm_main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a17d787..ddeb18a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -978,8 +978,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  		/* Check for overlaps */
>  		r = -EEXIST;
>  		kvm_for_each_memslot(slot, __kvm_memslots(kvm, as_id)) {
> -			if ((slot->id >= KVM_USER_MEM_SLOTS) ||
> -			    (slot->id == id))
> +			if (slot->id == id)
>  				continue;
>  			if (!((base_gfn + npages <= slot->base_gfn) ||
>  			      (base_gfn >= slot->base_gfn + slot->npages)))
> 

I wonder why the orginal patch explicitly mentions

"Prior to memory slot sorting this loop compared all of the user memory
slots... and skip comparison to private slots.".

Was/is there some use case where this was intended to work?

-- 

Thanks,

David