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

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

[Eric Biggers]

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

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

[Wanpeng Li]

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
>

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

[Wanpeng Li]

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

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

[Eric Biggers]

+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

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

[Eric Biggers]

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

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

[Eric Biggers]

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.

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

[Paolo Bonzini]

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