Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1609501 > unrolled thread
| Started by | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| First post | 2017-03-27 08:30 +0200 |
| Last post | 2017-04-03 14:40 +0200 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] KVM: mmu: Fix overlap with private memslots Wanpeng Li <kernellwp@gmail.com> - 2017-03-27 08:30 +0200
Re: [PATCH] KVM: mmu: Fix overlap with private memslots David Hildenbrand <david@redhat.com> - 2017-04-03 14:20 +0200
Re: [PATCH] KVM: mmu: Fix overlap with private memslots Pankaj Gupta <pagupta@redhat.com> - 2017-04-03 14:40 +0200
| From | Wanpeng Li <kernellwp@gmail.com> |
|---|---|
| Date | 2017-03-27 08:30 +0200 |
| Subject | [PATCH] KVM: mmu: Fix overlap with private memslots |
| Message-ID | <tpwka-3HQ-31@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | David Hildenbrand <david@redhat.com> |
|---|---|
| Date | 2017-04-03 14:20 +0200 |
| Message-ID | <ts97J-6L2-25@gated-at.bofh.it> |
| In reply to | #1609501 |
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
[toc] | [prev] | [next] | [standalone]
| From | Pankaj Gupta <pagupta@redhat.com> |
|---|---|
| Date | 2017-04-03 14:40 +0200 |
| Message-ID | <ts9r3-6Ro-5@gated-at.bofh.it> |
| In reply to | #1615152 |
> 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
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web