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