From: Wanpeng Li <[email protected]>
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 <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Radim Krčmář <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Alex Williamson <[email protected]>
Cc: <[email protected]> # v3.10+
Fixes: 5419369ed (KVM: Fix user memslot overlap check)
Signed-off-by: Wanpeng Li <[email protected]>
---
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)))
--
2.7.4
On 27.03.2017 08:23, Wanpeng Li wrote:
> From: Wanpeng Li <[email protected]>
>
> 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 <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Radim Krčmář <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> Cc: Alex Williamson <[email protected]>
> Cc: <[email protected]> # v3.10+
> Fixes: 5419369ed (KVM: Fix user memslot overlap check)
> Signed-off-by: Wanpeng Li <[email protected]>
> ---
> 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
> On 27.03.2017 08:23, Wanpeng Li wrote:
> > From: Wanpeng Li <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Paolo Bonzini <[email protected]>
> > Cc: Radim Krčmář <[email protected]>
> > Cc: Dmitry Vyukov <[email protected]>
> > Cc: Alex Williamson <[email protected]>
> > Cc: <[email protected]> # v3.10+
> > Fixes: 5419369ed (KVM: Fix user memslot overlap check)
> > Signed-off-by: Wanpeng Li <[email protected]>
> > ---
> > 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?
I also thought about this.
If this condition passes and it bypass check for slot overlap.
(slot->id >= KVM_USER_MEM_SLOTS)
But still wanted to know the case for which this check was there.
>
> --
>
> Thanks,
>
> David
>