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

[Pankaj Gupta]

> 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?

I also thought about this. 

If this condition passes and it bypass check for slot overlap.
(slot->id >= KVM_USER_MEM_SLOTS)

But still wanted to know the case for which this check was there. 

> 
> --
> 
> Thanks,
> 
> David
>