* [PATCH] KVM: mmu: Fix overlap with private memslots
@ 2017-03-27 6:23 Wanpeng Li
2017-04-03 12:10 ` David Hildenbrand
0 siblings, 1 reply; 3+ messages in thread
From: Wanpeng Li @ 2017-03-27 6:23 UTC (permalink / raw)
To: linux-kernel, kvm
Cc: Paolo Bonzini, Radim Krčmář, Wanpeng Li,
Dmitry Vyukov, Alex Williamson, # v3 . 10+
From: Wanpeng Li <wanpeng.li@hotmail.com>
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@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: <stable@vger.kernel.org> # v3.10+
Fixes: 5419369ed (KVM: Fix user memslot overlap check)
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.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 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: mmu: Fix overlap with private memslots
2017-03-27 6:23 [PATCH] KVM: mmu: Fix overlap with private memslots Wanpeng Li
@ 2017-04-03 12:10 ` David Hildenbrand
2017-04-03 12:30 ` Pankaj Gupta
0 siblings, 1 reply; 3+ messages in thread
From: David Hildenbrand @ 2017-04-03 12:10 UTC (permalink / raw)
To: Wanpeng Li, linux-kernel, kvm
Cc: Paolo Bonzini, Radim Krčmář, Wanpeng Li,
Dmitry Vyukov, Alex Williamson, # v3 . 10+
On 27.03.2017 08:23, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@hotmail.com>
>
> 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@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: <stable@vger.kernel.org> # v3.10+
> Fixes: 5419369ed (KVM: Fix user memslot overlap check)
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.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 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: mmu: Fix overlap with private memslots
2017-04-03 12:10 ` David Hildenbrand
@ 2017-04-03 12:30 ` Pankaj Gupta
0 siblings, 0 replies; 3+ messages in thread
From: Pankaj Gupta @ 2017-04-03 12:30 UTC (permalink / raw)
To: David Hildenbrand
Cc: Wanpeng Li, linux-kernel, kvm, Paolo Bonzini,
Radim Krčmář, Wanpeng Li, Dmitry Vyukov,
Alex Williamson, # v3 . 10+
> On 27.03.2017 08:23, Wanpeng Li wrote:
> > From: Wanpeng Li <wanpeng.li@hotmail.com>
> >
> > 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@google.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Cc: <stable@vger.kernel.org> # v3.10+
> > Fixes: 5419369ed (KVM: Fix user memslot overlap check)
> > Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.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 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-04-03 12:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-27 6:23 [PATCH] KVM: mmu: Fix overlap with private memslots Wanpeng Li
2017-04-03 12:10 ` David Hildenbrand
2017-04-03 12:30 ` Pankaj Gupta
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).