| Message ID | 1490595800-15060-1-git-send-email-wanpeng.li@hotmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27.03.2017 08:23, Wanpeng Li wrote: > From: Wanpeng Li <wanpeng.li@hotmail.com> > > 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@google.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Alex Williamson <alex.williamson@redhat.com> > Cc: <stable@vger.kernel.org> # v3.10+ > Fixes: 5419369ed (KVM: Fix user memslot overlap check) > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > --- > 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?
> On 27.03.2017 08:23, Wanpeng Li wrote: > > From: Wanpeng Li <wanpeng.li@hotmail.com> > > > > 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@google.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Radim Krčmář <rkrcmar@redhat.com> > > Cc: Dmitry Vyukov <dvyukov@google.com> > > Cc: Alex Williamson <alex.williamson@redhat.com> > > Cc: <stable@vger.kernel.org> # v3.10+ > > Fixes: 5419369ed (KVM: Fix user memslot overlap check) > > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com> > > --- > > 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 >
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)))