From: Eric Biggers <[email protected]>
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 <[email protected]>
Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots")
Cc: <[email protected]> # v2.6.25+
Signed-off-by: Eric Biggers <[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 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
2018-01-19 16:18 GMT+08:00 Eric Biggers <[email protected]>:
> From: Eric Biggers <[email protected]>
>
> 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 <[email protected]>
> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots")
> Cc: <[email protected]> # v2.6.25+
> Signed-off-by: Eric Biggers <[email protected]>
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
>
2018-01-19 17:01 GMT+08:00 Wanpeng Li <[email protected]>:
> 2018-01-19 16:18 GMT+08:00 Eric Biggers <[email protected]>:
>> From: Eric Biggers <[email protected]>
>>
>> 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 <[email protected]>
>> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots")
>> Cc: <[email protected]> # v2.6.25+
>> Signed-off-by: Eric Biggers <[email protected]>
>
> 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
>>
+Cc [email protected]
On Fri, Jan 19, 2018 at 05:03:47PM +0800, Wanpeng Li wrote:
> 2018-01-19 17:01 GMT+08:00 Wanpeng Li <[email protected]>:
> > 2018-01-19 16:18 GMT+08:00 Eric Biggers <[email protected]>:
> >> From: Eric Biggers <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots")
> >> Cc: <[email protected]> # v2.6.25+
> >> Signed-off-by: Eric Biggers <[email protected]>
> >
> > 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
On Fri, Jan 19, 2018 at 10:57:16AM -0800, Eric Biggers wrote:
> +Cc [email protected]
>
> On Fri, Jan 19, 2018 at 05:03:47PM +0800, Wanpeng Li wrote:
> > 2018-01-19 17:01 GMT+08:00 Wanpeng Li <[email protected]>:
> > > 2018-01-19 16:18 GMT+08:00 Eric Biggers <[email protected]>:
> > >> From: Eric Biggers <[email protected]>
> > >>
> > >> 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 <[email protected]>
> > >> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots")
> > >> Cc: <[email protected]> # v2.6.25+
> > >> Signed-off-by: Eric Biggers <[email protected]>
> > >
> > > 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
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 [email protected]
> >
> > On Fri, Jan 19, 2018 at 05:03:47PM +0800, Wanpeng Li wrote:
> > > 2018-01-19 17:01 GMT+08:00 Wanpeng Li <[email protected]>:
> > > > 2018-01-19 16:18 GMT+08:00 Eric Biggers <[email protected]>:
> > > >> From: Eric Biggers <[email protected]>
> > > >>
> > > >> 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 <[email protected]>
> > > >> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots")
> > > >> Cc: <[email protected]> # v2.6.25+
> > > >> Signed-off-by: Eric Biggers <[email protected]>
> > > >
> > > > 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.
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 [email protected]
>>>
>>> On Fri, Jan 19, 2018 at 05:03:47PM +0800, Wanpeng Li wrote:
>>>> 2018-01-19 17:01 GMT+08:00 Wanpeng Li <[email protected]>:
>>>>> 2018-01-19 16:18 GMT+08:00 Eric Biggers <[email protected]>:
>>>>>> From: Eric Biggers <[email protected]>
>>>>>>
>>>>>> 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 <[email protected]>
>>>>>> Fixes: e0d62c7f4860 ("KVM: Add kernel-internal memory slots")
>>>>>> Cc: <[email protected]> # v2.6.25+
>>>>>> Signed-off-by: Eric Biggers <[email protected]>
>>>>>
>>>>> 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