Re: [PATCH] KVM: prevent overlap between user and private memslots

[Eric Biggers]

+Cc alex.williamson@xxxxxxxxxx

On Fri, Jan 19, 2018 at 05:03:47PM +0800, Wanpeng Li wrote:
> 2018-01-19 17:01 GMT+08:00 Wanpeng Li <kernellwp@xxxxxxxxx>:
> > 2018-01-19 16:18 GMT+08:00 Eric Biggers <ebiggers3@xxxxxxxxx>:
> >> From: Eric Biggers <ebiggers@xxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxxx>
> >> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots")
> >> Cc: <stable@xxxxxxxxxxxxxxx> # v2.6.25+
> >> Signed-off-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> >
> > 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