Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Eric Biggers <ebiggers3@gmail.com>
To:     kvm@vger.kernel.org
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        =?UTF-8?q?Radim=20Kr=C4=8Dm=C3=A1=C5=99?= <rkrcmar@redhat.com>,
        linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
        Eric Biggers <ebiggers@google.com>, stable@vger.kernel.org
Subject: [PATCH] KVM: prevent overlap between user and private memslots
Date:   Fri, 19 Jan 2018 00:18:20 -0800
Message-Id: <20180119081820.30803-1-ebiggers3@gmail.com>
In-Reply-To: <94eb2c06f65e7ece95055cf1aafd@google.com>
References: <94eb2c06f65e7ece95055cf1aafd@google.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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