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