| Message ID | 20180119081820.30803-1-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2018-01-19 16:18 GMT+08:00 Eric Biggers <ebiggers3@gmail.com>: > From: Eric Biggers <ebiggers@google.com> > > Memslots must not overlap in guest physical memory, since otherwise some > guest physical addresses will not uniquely map to a memslot. Yet, the > overlap check in __kvm_set_memory_region() allows a memslot that > overlaps one of the "private" memslots, e.g. the memslot reserved for > the TSS on x86. > > This seems to be a very old bug that was introduced years ago when > private memory slots were first added. It seems that later refactoring > incorrectly assumed this bug was intentional and preserved it. > > Fix it by removing the loophole for private memslots, so we just check > for overlap against all memslots. > > This bug was found by syzkaller, which used a memslot overlap to make > pte_list_remove() be called for the wrong memslot, hitting a BUG(): > > pte_list_remove: 000000007185ed42 0->BUG > kernel BUG at arch/x86/kvm/mmu.c:1209! > [...] > RIP: 0010:pte_list_remove+0x107/0x110 arch/x86/kvm/mmu.c:1208 > [...] > Call Trace: > mmu_page_zap_pte+0x7e/0xd0 arch/x86/kvm/mmu.c:2499 > kvm_mmu_page_unlink_children arch/x86/kvm/mmu.c:2521 [inline] > kvm_mmu_prepare_zap_page+0x4f/0x340 arch/x86/kvm/mmu.c:2565 > kvm_zap_obsolete_pages arch/x86/kvm/mmu.c:5348 [inline] > kvm_mmu_invalidate_zap_all_pages+0xa6/0x100 arch/x86/kvm/mmu.c:5389 > kvm_mmu_notifier_release+0x4f/0x80 arch/x86/kvm/../../../virt/kvm/kvm_main.c:468 > __mmu_notifier_release+0x63/0x100 mm/mmu_notifier.c:75 > mmu_notifier_release include/linux/mmu_notifier.h:244 [inline] > exit_mmap+0x160/0x170 mm/mmap.c:3009 > __mmput kernel/fork.c:966 [inline] > mmput+0x44/0xd0 kernel/fork.c:987 > exit_mm kernel/exit.c:544 [inline] > do_exit+0x24a/0xb50 kernel/exit.c:856 > do_group_exit+0x34/0xb0 kernel/exit.c:972 > SYSC_exit_group kernel/exit.c:983 [inline] > SyS_exit_group+0xb/0x10 kernel/exit.c:981 > entry_SYSCALL_64_fastpath+0x1e/0x8b > > Reproducer: > > #include <fcntl.h> > #include <linux/kvm.h> > #include <sys/ioctl.h> > > int main() > { > static char buf[4096*3] __attribute__((aligned(4096))); > int kvm, vm, cpu; > struct kvm_mp_state mp_state = { KVM_MP_STATE_SIPI_RECEIVED }; > struct kvm_userspace_memory_region memreg = { > .memory_size = sizeof(buf), > .userspace_addr = (__u64)buf, > }; > > kvm = open("/dev/kvm", O_RDWR); > vm = ioctl(kvm, KVM_CREATE_VM, 0); > ioctl(vm, KVM_CREATE_IRQCHIP); > cpu = ioctl(vm, KVM_CREATE_VCPU, 0); > ioctl(cpu, KVM_SET_MP_STATE, &mp_state); > ioctl(vm, KVM_SET_TSS_ADDR, 0); > ioctl(cpu, KVM_RUN, 0); > ioctl(vm, KVM_SET_USER_MEMORY_REGION, &memreg); > } > > Reported-by: syzbot <syzkaller@googlegroups.com> > Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots") > Cc: <stable@vger.kernel.org> # v2.6.25+ > Signed-off-by: Eric Biggers <ebiggers@google.com> Please refer to this one. https://patchwork.kernel.org/patch/9645377/ Regards, Wanpeng Li > --- > 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 210bf820385a..e536977e7b6d 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -974,8 +974,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))) > -- > 2.16.0 >
2018-01-19 17:01 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>: > 2018-01-19 16:18 GMT+08:00 Eric Biggers <ebiggers3@gmail.com>: >> From: Eric Biggers <ebiggers@google.com> >> >> Memslots must not overlap in guest physical memory, since otherwise some >> guest physical addresses will not uniquely map to a memslot. Yet, the >> overlap check in __kvm_set_memory_region() allows a memslot that >> overlaps one of the "private" memslots, e.g. the memslot reserved for >> the TSS on x86. >> >> This seems to be a very old bug that was introduced years ago when >> private memory slots were first added. It seems that later refactoring >> incorrectly assumed this bug was intentional and preserved it. >> >> Fix it by removing the loophole for private memslots, so we just check >> for overlap against all memslots. >> >> This bug was found by syzkaller, which used a memslot overlap to make >> pte_list_remove() be called for the wrong memslot, hitting a BUG(): >> >> pte_list_remove: 000000007185ed42 0->BUG >> kernel BUG at arch/x86/kvm/mmu.c:1209! >> [...] >> RIP: 0010:pte_list_remove+0x107/0x110 arch/x86/kvm/mmu.c:1208 >> [...] >> Call Trace: >> mmu_page_zap_pte+0x7e/0xd0 arch/x86/kvm/mmu.c:2499 >> kvm_mmu_page_unlink_children arch/x86/kvm/mmu.c:2521 [inline] >> kvm_mmu_prepare_zap_page+0x4f/0x340 arch/x86/kvm/mmu.c:2565 >> kvm_zap_obsolete_pages arch/x86/kvm/mmu.c:5348 [inline] >> kvm_mmu_invalidate_zap_all_pages+0xa6/0x100 arch/x86/kvm/mmu.c:5389 >> kvm_mmu_notifier_release+0x4f/0x80 arch/x86/kvm/../../../virt/kvm/kvm_main.c:468 >> __mmu_notifier_release+0x63/0x100 mm/mmu_notifier.c:75 >> mmu_notifier_release include/linux/mmu_notifier.h:244 [inline] >> exit_mmap+0x160/0x170 mm/mmap.c:3009 >> __mmput kernel/fork.c:966 [inline] >> mmput+0x44/0xd0 kernel/fork.c:987 >> exit_mm kernel/exit.c:544 [inline] >> do_exit+0x24a/0xb50 kernel/exit.c:856 >> do_group_exit+0x34/0xb0 kernel/exit.c:972 >> SYSC_exit_group kernel/exit.c:983 [inline] >> SyS_exit_group+0xb/0x10 kernel/exit.c:981 >> entry_SYSCALL_64_fastpath+0x1e/0x8b >> >> Reproducer: >> >> #include <fcntl.h> >> #include <linux/kvm.h> >> #include <sys/ioctl.h> >> >> int main() >> { >> static char buf[4096*3] __attribute__((aligned(4096))); >> int kvm, vm, cpu; >> struct kvm_mp_state mp_state = { KVM_MP_STATE_SIPI_RECEIVED }; >> struct kvm_userspace_memory_region memreg = { >> .memory_size = sizeof(buf), >> .userspace_addr = (__u64)buf, >> }; >> >> kvm = open("/dev/kvm", O_RDWR); >> vm = ioctl(kvm, KVM_CREATE_VM, 0); >> ioctl(vm, KVM_CREATE_IRQCHIP); >> cpu = ioctl(vm, KVM_CREATE_VCPU, 0); >> ioctl(cpu, KVM_SET_MP_STATE, &mp_state); >> ioctl(vm, KVM_SET_TSS_ADDR, 0); >> ioctl(cpu, KVM_RUN, 0); >> ioctl(vm, KVM_SET_USER_MEMORY_REGION, &memreg); >> } >> >> Reported-by: syzbot <syzkaller@googlegroups.com> >> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots") >> Cc: <stable@vger.kernel.org> # v2.6.25+ >> Signed-off-by: Eric Biggers <ebiggers@google.com> > > Please refer to this one. https://patchwork.kernel.org/patch/9645377/ https://lkml.org/lkml/2017/3/27/57 > > Regards, > Wanpeng Li > >> --- >> 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 210bf820385a..e536977e7b6d 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -974,8 +974,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))) >> -- >> 2.16.0 >>
+Cc alex.williamson@redhat.com On Fri, Jan 19, 2018 at 05:03:47PM +0800, Wanpeng Li wrote: > 2018-01-19 17:01 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>: > > 2018-01-19 16:18 GMT+08:00 Eric Biggers <ebiggers3@gmail.com>: > >> From: Eric Biggers <ebiggers@google.com> > >> > >> Memslots must not overlap in guest physical memory, since otherwise some > >> guest physical addresses will not uniquely map to a memslot. Yet, the > >> overlap check in __kvm_set_memory_region() allows a memslot that > >> overlaps one of the "private" memslots, e.g. the memslot reserved for > >> the TSS on x86. > >> > >> This seems to be a very old bug that was introduced years ago when > >> private memory slots were first added. It seems that later refactoring > >> incorrectly assumed this bug was intentional and preserved it. > >> > >> Fix it by removing the loophole for private memslots, so we just check > >> for overlap against all memslots. > >> > >> This bug was found by syzkaller, which used a memslot overlap to make > >> pte_list_remove() be called for the wrong memslot, hitting a BUG(): > >> > >> pte_list_remove: 000000007185ed42 0->BUG > >> kernel BUG at arch/x86/kvm/mmu.c:1209! > >> [...] > >> RIP: 0010:pte_list_remove+0x107/0x110 arch/x86/kvm/mmu.c:1208 > >> [...] > >> Call Trace: > >> mmu_page_zap_pte+0x7e/0xd0 arch/x86/kvm/mmu.c:2499 > >> kvm_mmu_page_unlink_children arch/x86/kvm/mmu.c:2521 [inline] > >> kvm_mmu_prepare_zap_page+0x4f/0x340 arch/x86/kvm/mmu.c:2565 > >> kvm_zap_obsolete_pages arch/x86/kvm/mmu.c:5348 [inline] > >> kvm_mmu_invalidate_zap_all_pages+0xa6/0x100 arch/x86/kvm/mmu.c:5389 > >> kvm_mmu_notifier_release+0x4f/0x80 arch/x86/kvm/../../../virt/kvm/kvm_main.c:468 > >> __mmu_notifier_release+0x63/0x100 mm/mmu_notifier.c:75 > >> mmu_notifier_release include/linux/mmu_notifier.h:244 [inline] > >> exit_mmap+0x160/0x170 mm/mmap.c:3009 > >> __mmput kernel/fork.c:966 [inline] > >> mmput+0x44/0xd0 kernel/fork.c:987 > >> exit_mm kernel/exit.c:544 [inline] > >> do_exit+0x24a/0xb50 kernel/exit.c:856 > >> do_group_exit+0x34/0xb0 kernel/exit.c:972 > >> SYSC_exit_group kernel/exit.c:983 [inline] > >> SyS_exit_group+0xb/0x10 kernel/exit.c:981 > >> entry_SYSCALL_64_fastpath+0x1e/0x8b > >> > >> Reproducer: > >> > >> #include <fcntl.h> > >> #include <linux/kvm.h> > >> #include <sys/ioctl.h> > >> > >> int main() > >> { > >> static char buf[4096*3] __attribute__((aligned(4096))); > >> int kvm, vm, cpu; > >> struct kvm_mp_state mp_state = { KVM_MP_STATE_SIPI_RECEIVED }; > >> struct kvm_userspace_memory_region memreg = { > >> .memory_size = sizeof(buf), > >> .userspace_addr = (__u64)buf, > >> }; > >> > >> kvm = open("/dev/kvm", O_RDWR); > >> vm = ioctl(kvm, KVM_CREATE_VM, 0); > >> ioctl(vm, KVM_CREATE_IRQCHIP); > >> cpu = ioctl(vm, KVM_CREATE_VCPU, 0); > >> ioctl(cpu, KVM_SET_MP_STATE, &mp_state); > >> ioctl(vm, KVM_SET_TSS_ADDR, 0); > >> ioctl(cpu, KVM_RUN, 0); > >> ioctl(vm, KVM_SET_USER_MEMORY_REGION, &memreg); > >> } > >> > >> Reported-by: syzbot <syzkaller@googlegroups.com> > >> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots") > >> Cc: <stable@vger.kernel.org> # v2.6.25+ > >> Signed-off-by: Eric Biggers <ebiggers@google.com> > > > > Please refer to this one. https://patchwork.kernel.org/patch/9645377/ > > https://lkml.org/lkml/2017/3/27/57 > Ah, so this was reported before, and you sent the same fix. Well, it was never applied, so the bug is still there, and anyone who can use /dev/kvm can trigger it. So one of these patches needs to be applied, unless there is a better fix. I don't agree with the "Fixes:" line in your version of the patch. The bug was actually there prior to 5419369ed, which might explain why that commit seemed to preserve the behavior intentionally. (Note that KVM_MEMORY_SLOTS did not include the private memory slots; it was later renamed to KVM_USER_MEM_SLOTS.) Eric
On Fri, Jan 19, 2018 at 10:57:16AM -0800, Eric Biggers wrote: > +Cc alex.williamson@redhat.com > > On Fri, Jan 19, 2018 at 05:03:47PM +0800, Wanpeng Li wrote: > > 2018-01-19 17:01 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>: > > > 2018-01-19 16:18 GMT+08:00 Eric Biggers <ebiggers3@gmail.com>: > > >> From: Eric Biggers <ebiggers@google.com> > > >> > > >> Memslots must not overlap in guest physical memory, since otherwise some > > >> guest physical addresses will not uniquely map to a memslot. Yet, the > > >> overlap check in __kvm_set_memory_region() allows a memslot that > > >> overlaps one of the "private" memslots, e.g. the memslot reserved for > > >> the TSS on x86. > > >> > > >> This seems to be a very old bug that was introduced years ago when > > >> private memory slots were first added. It seems that later refactoring > > >> incorrectly assumed this bug was intentional and preserved it. > > >> > > >> Fix it by removing the loophole for private memslots, so we just check > > >> for overlap against all memslots. > > >> > > >> This bug was found by syzkaller, which used a memslot overlap to make > > >> pte_list_remove() be called for the wrong memslot, hitting a BUG(): > > >> > > >> pte_list_remove: 000000007185ed42 0->BUG > > >> kernel BUG at arch/x86/kvm/mmu.c:1209! > > >> [...] > > >> RIP: 0010:pte_list_remove+0x107/0x110 arch/x86/kvm/mmu.c:1208 > > >> [...] > > >> Call Trace: > > >> mmu_page_zap_pte+0x7e/0xd0 arch/x86/kvm/mmu.c:2499 > > >> kvm_mmu_page_unlink_children arch/x86/kvm/mmu.c:2521 [inline] > > >> kvm_mmu_prepare_zap_page+0x4f/0x340 arch/x86/kvm/mmu.c:2565 > > >> kvm_zap_obsolete_pages arch/x86/kvm/mmu.c:5348 [inline] > > >> kvm_mmu_invalidate_zap_all_pages+0xa6/0x100 arch/x86/kvm/mmu.c:5389 > > >> kvm_mmu_notifier_release+0x4f/0x80 arch/x86/kvm/../../../virt/kvm/kvm_main.c:468 > > >> __mmu_notifier_release+0x63/0x100 mm/mmu_notifier.c:75 > > >> mmu_notifier_release include/linux/mmu_notifier.h:244 [inline] > > >> exit_mmap+0x160/0x170 mm/mmap.c:3009 > > >> __mmput kernel/fork.c:966 [inline] > > >> mmput+0x44/0xd0 kernel/fork.c:987 > > >> exit_mm kernel/exit.c:544 [inline] > > >> do_exit+0x24a/0xb50 kernel/exit.c:856 > > >> do_group_exit+0x34/0xb0 kernel/exit.c:972 > > >> SYSC_exit_group kernel/exit.c:983 [inline] > > >> SyS_exit_group+0xb/0x10 kernel/exit.c:981 > > >> entry_SYSCALL_64_fastpath+0x1e/0x8b > > >> > > >> Reproducer: > > >> > > >> #include <fcntl.h> > > >> #include <linux/kvm.h> > > >> #include <sys/ioctl.h> > > >> > > >> int main() > > >> { > > >> static char buf[4096*3] __attribute__((aligned(4096))); > > >> int kvm, vm, cpu; > > >> struct kvm_mp_state mp_state = { KVM_MP_STATE_SIPI_RECEIVED }; > > >> struct kvm_userspace_memory_region memreg = { > > >> .memory_size = sizeof(buf), > > >> .userspace_addr = (__u64)buf, > > >> }; > > >> > > >> kvm = open("/dev/kvm", O_RDWR); > > >> vm = ioctl(kvm, KVM_CREATE_VM, 0); > > >> ioctl(vm, KVM_CREATE_IRQCHIP); > > >> cpu = ioctl(vm, KVM_CREATE_VCPU, 0); > > >> ioctl(cpu, KVM_SET_MP_STATE, &mp_state); > > >> ioctl(vm, KVM_SET_TSS_ADDR, 0); > > >> ioctl(cpu, KVM_RUN, 0); > > >> ioctl(vm, KVM_SET_USER_MEMORY_REGION, &memreg); > > >> } > > >> > > >> Reported-by: syzbot <syzkaller@googlegroups.com> > > >> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots") > > >> Cc: <stable@vger.kernel.org> # v2.6.25+ > > >> Signed-off-by: Eric Biggers <ebiggers@google.com> > > > > > > Please refer to this one. https://patchwork.kernel.org/patch/9645377/ > > > > https://lkml.org/lkml/2017/3/27/57 > > > > Ah, so this was reported before, and you sent the same fix. Well, it was never > applied, so the bug is still there, and anyone who can use /dev/kvm can trigger > it. So one of these patches needs to be applied, unless there is a better fix. > > I don't agree with the "Fixes:" line in your version of the patch. The bug was > actually there prior to 5419369ed, which might explain why that commit seemed to > preserve the behavior intentionally. (Note that KVM_MEMORY_SLOTS did not > include the private memory slots; it was later renamed to KVM_USER_MEM_SLOTS.) > > Eric Ping. Paolo or Radim, can you please consider applying one of these patches? Eric
On Tue, Jan 30, 2018 at 12:49:21PM -0800, Eric Biggers wrote: > On Fri, Jan 19, 2018 at 10:57:16AM -0800, Eric Biggers wrote: > > +Cc alex.williamson@redhat.com > > > > On Fri, Jan 19, 2018 at 05:03:47PM +0800, Wanpeng Li wrote: > > > 2018-01-19 17:01 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>: > > > > 2018-01-19 16:18 GMT+08:00 Eric Biggers <ebiggers3@gmail.com>: > > > >> From: Eric Biggers <ebiggers@google.com> > > > >> > > > >> Memslots must not overlap in guest physical memory, since otherwise some > > > >> guest physical addresses will not uniquely map to a memslot. Yet, the > > > >> overlap check in __kvm_set_memory_region() allows a memslot that > > > >> overlaps one of the "private" memslots, e.g. the memslot reserved for > > > >> the TSS on x86. > > > >> > > > >> This seems to be a very old bug that was introduced years ago when > > > >> private memory slots were first added. It seems that later refactoring > > > >> incorrectly assumed this bug was intentional and preserved it. > > > >> > > > >> Fix it by removing the loophole for private memslots, so we just check > > > >> for overlap against all memslots. > > > >> > > > >> This bug was found by syzkaller, which used a memslot overlap to make > > > >> pte_list_remove() be called for the wrong memslot, hitting a BUG(): > > > >> > > > >> pte_list_remove: 000000007185ed42 0->BUG > > > >> kernel BUG at arch/x86/kvm/mmu.c:1209! > > > >> [...] > > > >> RIP: 0010:pte_list_remove+0x107/0x110 arch/x86/kvm/mmu.c:1208 > > > >> [...] > > > >> Call Trace: > > > >> mmu_page_zap_pte+0x7e/0xd0 arch/x86/kvm/mmu.c:2499 > > > >> kvm_mmu_page_unlink_children arch/x86/kvm/mmu.c:2521 [inline] > > > >> kvm_mmu_prepare_zap_page+0x4f/0x340 arch/x86/kvm/mmu.c:2565 > > > >> kvm_zap_obsolete_pages arch/x86/kvm/mmu.c:5348 [inline] > > > >> kvm_mmu_invalidate_zap_all_pages+0xa6/0x100 arch/x86/kvm/mmu.c:5389 > > > >> kvm_mmu_notifier_release+0x4f/0x80 arch/x86/kvm/../../../virt/kvm/kvm_main.c:468 > > > >> __mmu_notifier_release+0x63/0x100 mm/mmu_notifier.c:75 > > > >> mmu_notifier_release include/linux/mmu_notifier.h:244 [inline] > > > >> exit_mmap+0x160/0x170 mm/mmap.c:3009 > > > >> __mmput kernel/fork.c:966 [inline] > > > >> mmput+0x44/0xd0 kernel/fork.c:987 > > > >> exit_mm kernel/exit.c:544 [inline] > > > >> do_exit+0x24a/0xb50 kernel/exit.c:856 > > > >> do_group_exit+0x34/0xb0 kernel/exit.c:972 > > > >> SYSC_exit_group kernel/exit.c:983 [inline] > > > >> SyS_exit_group+0xb/0x10 kernel/exit.c:981 > > > >> entry_SYSCALL_64_fastpath+0x1e/0x8b > > > >> > > > >> Reproducer: > > > >> > > > >> #include <fcntl.h> > > > >> #include <linux/kvm.h> > > > >> #include <sys/ioctl.h> > > > >> > > > >> int main() > > > >> { > > > >> static char buf[4096*3] __attribute__((aligned(4096))); > > > >> int kvm, vm, cpu; > > > >> struct kvm_mp_state mp_state = { KVM_MP_STATE_SIPI_RECEIVED }; > > > >> struct kvm_userspace_memory_region memreg = { > > > >> .memory_size = sizeof(buf), > > > >> .userspace_addr = (__u64)buf, > > > >> }; > > > >> > > > >> kvm = open("/dev/kvm", O_RDWR); > > > >> vm = ioctl(kvm, KVM_CREATE_VM, 0); > > > >> ioctl(vm, KVM_CREATE_IRQCHIP); > > > >> cpu = ioctl(vm, KVM_CREATE_VCPU, 0); > > > >> ioctl(cpu, KVM_SET_MP_STATE, &mp_state); > > > >> ioctl(vm, KVM_SET_TSS_ADDR, 0); > > > >> ioctl(cpu, KVM_RUN, 0); > > > >> ioctl(vm, KVM_SET_USER_MEMORY_REGION, &memreg); > > > >> } > > > >> > > > >> Reported-by: syzbot <syzkaller@googlegroups.com> > > > >> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots") > > > >> Cc: <stable@vger.kernel.org> # v2.6.25+ > > > >> Signed-off-by: Eric Biggers <ebiggers@google.com> > > > > > > > > Please refer to this one. https://patchwork.kernel.org/patch/9645377/ > > > > > > https://lkml.org/lkml/2017/3/27/57 > > > > > > > Ah, so this was reported before, and you sent the same fix. Well, it was never > > applied, so the bug is still there, and anyone who can use /dev/kvm can trigger > > it. So one of these patches needs to be applied, unless there is a better fix. > > > > I don't agree with the "Fixes:" line in your version of the patch. The bug was > > actually there prior to 5419369ed, which might explain why that commit seemed to > > preserve the behavior intentionally. (Note that KVM_MEMORY_SLOTS did not > > include the private memory slots; it was later renamed to KVM_USER_MEM_SLOTS.) > > > > Eric > > Ping. Paolo or Radim, can you please consider applying one of these patches? > > Eric Ping.
On 13/02/2018 04:38, Eric Biggers wrote: > On Tue, Jan 30, 2018 at 12:49:21PM -0800, Eric Biggers wrote: >> On Fri, Jan 19, 2018 at 10:57:16AM -0800, Eric Biggers wrote: >>> +Cc alex.williamson@redhat.com >>> >>> On Fri, Jan 19, 2018 at 05:03:47PM +0800, Wanpeng Li wrote: >>>> 2018-01-19 17:01 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>: >>>>> 2018-01-19 16:18 GMT+08:00 Eric Biggers <ebiggers3@gmail.com>: >>>>>> From: Eric Biggers <ebiggers@google.com> >>>>>> >>>>>> Memslots must not overlap in guest physical memory, since otherwise some >>>>>> guest physical addresses will not uniquely map to a memslot. Yet, the >>>>>> overlap check in __kvm_set_memory_region() allows a memslot that >>>>>> overlaps one of the "private" memslots, e.g. the memslot reserved for >>>>>> the TSS on x86. >>>>>> >>>>>> This seems to be a very old bug that was introduced years ago when >>>>>> private memory slots were first added. It seems that later refactoring >>>>>> incorrectly assumed this bug was intentional and preserved it. >>>>>> >>>>>> Fix it by removing the loophole for private memslots, so we just check >>>>>> for overlap against all memslots. >>>>>> >>>>>> This bug was found by syzkaller, which used a memslot overlap to make >>>>>> pte_list_remove() be called for the wrong memslot, hitting a BUG(): >>>>>> >>>>>> pte_list_remove: 000000007185ed42 0->BUG >>>>>> kernel BUG at arch/x86/kvm/mmu.c:1209! >>>>>> [...] >>>>>> RIP: 0010:pte_list_remove+0x107/0x110 arch/x86/kvm/mmu.c:1208 >>>>>> [...] >>>>>> Call Trace: >>>>>> mmu_page_zap_pte+0x7e/0xd0 arch/x86/kvm/mmu.c:2499 >>>>>> kvm_mmu_page_unlink_children arch/x86/kvm/mmu.c:2521 [inline] >>>>>> kvm_mmu_prepare_zap_page+0x4f/0x340 arch/x86/kvm/mmu.c:2565 >>>>>> kvm_zap_obsolete_pages arch/x86/kvm/mmu.c:5348 [inline] >>>>>> kvm_mmu_invalidate_zap_all_pages+0xa6/0x100 arch/x86/kvm/mmu.c:5389 >>>>>> kvm_mmu_notifier_release+0x4f/0x80 arch/x86/kvm/../../../virt/kvm/kvm_main.c:468 >>>>>> __mmu_notifier_release+0x63/0x100 mm/mmu_notifier.c:75 >>>>>> mmu_notifier_release include/linux/mmu_notifier.h:244 [inline] >>>>>> exit_mmap+0x160/0x170 mm/mmap.c:3009 >>>>>> __mmput kernel/fork.c:966 [inline] >>>>>> mmput+0x44/0xd0 kernel/fork.c:987 >>>>>> exit_mm kernel/exit.c:544 [inline] >>>>>> do_exit+0x24a/0xb50 kernel/exit.c:856 >>>>>> do_group_exit+0x34/0xb0 kernel/exit.c:972 >>>>>> SYSC_exit_group kernel/exit.c:983 [inline] >>>>>> SyS_exit_group+0xb/0x10 kernel/exit.c:981 >>>>>> entry_SYSCALL_64_fastpath+0x1e/0x8b >>>>>> >>>>>> Reproducer: >>>>>> >>>>>> #include <fcntl.h> >>>>>> #include <linux/kvm.h> >>>>>> #include <sys/ioctl.h> >>>>>> >>>>>> int main() >>>>>> { >>>>>> static char buf[4096*3] __attribute__((aligned(4096))); >>>>>> int kvm, vm, cpu; >>>>>> struct kvm_mp_state mp_state = { KVM_MP_STATE_SIPI_RECEIVED }; >>>>>> struct kvm_userspace_memory_region memreg = { >>>>>> .memory_size = sizeof(buf), >>>>>> .userspace_addr = (__u64)buf, >>>>>> }; >>>>>> >>>>>> kvm = open("/dev/kvm", O_RDWR); >>>>>> vm = ioctl(kvm, KVM_CREATE_VM, 0); >>>>>> ioctl(vm, KVM_CREATE_IRQCHIP); >>>>>> cpu = ioctl(vm, KVM_CREATE_VCPU, 0); >>>>>> ioctl(cpu, KVM_SET_MP_STATE, &mp_state); >>>>>> ioctl(vm, KVM_SET_TSS_ADDR, 0); >>>>>> ioctl(cpu, KVM_RUN, 0); >>>>>> ioctl(vm, KVM_SET_USER_MEMORY_REGION, &memreg); >>>>>> } >>>>>> >>>>>> Reported-by: syzbot <syzkaller@googlegroups.com> >>>>>> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots") >>>>>> Cc: <stable@vger.kernel.org> # v2.6.25+ >>>>>> Signed-off-by: Eric Biggers <ebiggers@google.com> >>>>> >>>>> Please refer to this one. https://patchwork.kernel.org/patch/9645377/ >>>> >>>> https://lkml.org/lkml/2017/3/27/57 >>>> >>> >>> Ah, so this was reported before, and you sent the same fix. Well, it was never >>> applied, so the bug is still there, and anyone who can use /dev/kvm can trigger >>> it. So one of these patches needs to be applied, unless there is a better fix. >>> >>> I don't agree with the "Fixes:" line in your version of the patch. The bug was >>> actually there prior to 5419369ed, which might explain why that commit seemed to >>> preserve the behavior intentionally. (Note that KVM_MEMORY_SLOTS did not >>> include the private memory slots; it was later renamed to KVM_USER_MEM_SLOTS.) >>> >>> Eric >> >> Ping. Paolo or Radim, can you please consider applying one of these patches? Applied, thanks. Paolo
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 210bf820385a..e536977e7b6d 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -974,8 +974,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)))