Hello,
syzbot found the following crash on:
HEAD commit: 88b95ef4c780 kmsan: use MSan assembly instrumentation
git tree: https://github.com/google/kmsan.git/master
console output: https://syzkaller.appspot.com/x/log.txt?x=12505e33400000
kernel config: https://syzkaller.appspot.com/x/.config?x=8df5fc509a1b351b
dashboard link: https://syzkaller.appspot.com/bug?extid=ded1696f6b50b615b630
compiler: clang version 8.0.0 (trunk 343298)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15ce62f5400000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=174efca3400000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: [email protected]
==================================================================
BUG: KMSAN: kernel-infoleak in __copy_to_user include/linux/uaccess.h:121
[inline]
BUG: KMSAN: kernel-infoleak in __kvm_write_guest_page
arch/x86/kvm/../../../virt/kvm/kvm_main.c:1849 [inline]
BUG: KMSAN: kernel-infoleak in kvm_vcpu_write_guest_page+0x39a/0x510
arch/x86/kvm/../../../virt/kvm/kvm_main.c:1870
CPU: 0 PID: 7918 Comm: syz-executor542 Not tainted 4.19.0+ #77
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x32d/0x480 lib/dump_stack.c:113
kmsan_report+0x1a2/0x2e0 mm/kmsan/kmsan.c:911
kmsan_internal_check_memory+0x34c/0x430 mm/kmsan/kmsan.c:991
kmsan_copy_to_user+0x85/0xe0 mm/kmsan/kmsan_hooks.c:552
__copy_to_user include/linux/uaccess.h:121 [inline]
__kvm_write_guest_page arch/x86/kvm/../../../virt/kvm/kvm_main.c:1849
[inline]
kvm_vcpu_write_guest_page+0x39a/0x510
arch/x86/kvm/../../../virt/kvm/kvm_main.c:1870
nested_release_vmcs12 arch/x86/kvm/vmx.c:8441 [inline]
handle_vmptrld+0x2384/0x26b0 arch/x86/kvm/vmx.c:8907
vmx_handle_exit+0x1e81/0xbac0 arch/x86/kvm/vmx.c:10128
vcpu_enter_guest arch/x86/kvm/x86.c:7667 [inline]
vcpu_run arch/x86/kvm/x86.c:7730 [inline]
kvm_arch_vcpu_ioctl_run+0xac32/0x11d80 arch/x86/kvm/x86.c:7930
kvm_vcpu_ioctl+0xfb1/0x1f90 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2590
do_vfs_ioctl+0xf77/0x2d30 fs/ioctl.c:46
ksys_ioctl fs/ioctl.c:702 [inline]
__do_sys_ioctl fs/ioctl.c:709 [inline]
__se_sys_ioctl+0x1da/0x270 fs/ioctl.c:707
__x64_sys_ioctl+0x4a/0x70 fs/ioctl.c:707
do_syscall_64+0xcf/0x110 arch/x86/entry/common.c:291
entry_SYSCALL_64_after_hwframe+0x63/0xe7
RIP: 0033:0x44b6e9
Code: e8 dc e6 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 2b ff fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f096b292ce8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00000000006e3c48 RCX: 000000000044b6e9
RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000005
RBP: 00000000006e3c40 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000206 R12: 00000000006e3c4c
R13: 00007ffd978aeb2f R14: 00007f096b2939c0 R15: 00000000006e3d4c
Uninit was created at:
kmsan_save_stack_with_flags mm/kmsan/kmsan.c:252 [inline]
kmsan_internal_poison_shadow+0xc8/0x1e0 mm/kmsan/kmsan.c:177
kmsan_kmalloc+0x98/0x110 mm/kmsan/kmsan_hooks.c:104
__kmalloc+0x14c/0x4d0 mm/slub.c:3789
kmalloc include/linux/slab.h:518 [inline]
enter_vmx_operation+0x980/0x1a90 arch/x86/kvm/vmx.c:8278
vmx_set_nested_state+0xc3a/0x1530 arch/x86/kvm/vmx.c:14045
kvm_arch_vcpu_ioctl+0x4fc9/0x73a0 arch/x86/kvm/x86.c:4057
kvm_vcpu_ioctl+0xca3/0x1f90 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2742
do_vfs_ioctl+0xf77/0x2d30 fs/ioctl.c:46
ksys_ioctl fs/ioctl.c:702 [inline]
__do_sys_ioctl fs/ioctl.c:709 [inline]
__se_sys_ioctl+0x1da/0x270 fs/ioctl.c:707
__x64_sys_ioctl+0x4a/0x70 fs/ioctl.c:707
do_syscall_64+0xcf/0x110 arch/x86/entry/common.c:291
entry_SYSCALL_64_after_hwframe+0x63/0xe7
Bytes 1000-4095 of 4096 are uninitialized
Memory access of size 4096 starts at ffff8801b5157000
==================================================================
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at [email protected].
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
On Wed, Nov 7, 2018 at 2:38 AM syzbot
<[email protected]> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 88b95ef4c780 kmsan: use MSan assembly instrumentation
> git tree: https://github.com/google/kmsan.git/master
> console output: https://syzkaller.appspot.com/x/log.txt?x=12505e33400000
> kernel config: https://syzkaller.appspot.com/x/.config?x=8df5fc509a1b351b
> dashboard link: https://syzkaller.appspot.com/bug?extid=ded1696f6b50b615b630
> compiler: clang version 8.0.0 (trunk 343298)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15ce62f5400000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=174efca3400000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: [email protected]
>
> ==================================================================
> BUG: KMSAN: kernel-infoleak in __copy_to_user include/linux/uaccess.h:121
> [inline]
> BUG: KMSAN: kernel-infoleak in __kvm_write_guest_page
> arch/x86/kvm/../../../virt/kvm/kvm_main.c:1849 [inline]
> BUG: KMSAN: kernel-infoleak in kvm_vcpu_write_guest_page+0x39a/0x510
> arch/x86/kvm/../../../virt/kvm/kvm_main.c:1870
> CPU: 0 PID: 7918 Comm: syz-executor542 Not tainted 4.19.0+ #77
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x32d/0x480 lib/dump_stack.c:113
> kmsan_report+0x1a2/0x2e0 mm/kmsan/kmsan.c:911
> kmsan_internal_check_memory+0x34c/0x430 mm/kmsan/kmsan.c:991
> kmsan_copy_to_user+0x85/0xe0 mm/kmsan/kmsan_hooks.c:552
> __copy_to_user include/linux/uaccess.h:121 [inline]
> __kvm_write_guest_page arch/x86/kvm/../../../virt/kvm/kvm_main.c:1849
> [inline]
> kvm_vcpu_write_guest_page+0x39a/0x510
> arch/x86/kvm/../../../virt/kvm/kvm_main.c:1870
> nested_release_vmcs12 arch/x86/kvm/vmx.c:8441 [inline]
> handle_vmptrld+0x2384/0x26b0 arch/x86/kvm/vmx.c:8907
> vmx_handle_exit+0x1e81/0xbac0 arch/x86/kvm/vmx.c:10128
> vcpu_enter_guest arch/x86/kvm/x86.c:7667 [inline]
> vcpu_run arch/x86/kvm/x86.c:7730 [inline]
> kvm_arch_vcpu_ioctl_run+0xac32/0x11d80 arch/x86/kvm/x86.c:7930
> kvm_vcpu_ioctl+0xfb1/0x1f90 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2590
> do_vfs_ioctl+0xf77/0x2d30 fs/ioctl.c:46
> ksys_ioctl fs/ioctl.c:702 [inline]
> __do_sys_ioctl fs/ioctl.c:709 [inline]
> __se_sys_ioctl+0x1da/0x270 fs/ioctl.c:707
> __x64_sys_ioctl+0x4a/0x70 fs/ioctl.c:707
> do_syscall_64+0xcf/0x110 arch/x86/entry/common.c:291
> entry_SYSCALL_64_after_hwframe+0x63/0xe7
> RIP: 0033:0x44b6e9
> Code: e8 dc e6 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
> ff 0f 83 2b ff fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007f096b292ce8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 00000000006e3c48 RCX: 000000000044b6e9
> RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000005
> RBP: 00000000006e3c40 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000206 R12: 00000000006e3c4c
> R13: 00007ffd978aeb2f R14: 00007f096b2939c0 R15: 00000000006e3d4c
>
> Uninit was created at:
> kmsan_save_stack_with_flags mm/kmsan/kmsan.c:252 [inline]
> kmsan_internal_poison_shadow+0xc8/0x1e0 mm/kmsan/kmsan.c:177
> kmsan_kmalloc+0x98/0x110 mm/kmsan/kmsan_hooks.c:104
> __kmalloc+0x14c/0x4d0 mm/slub.c:3789
> kmalloc include/linux/slab.h:518 [inline]
> enter_vmx_operation+0x980/0x1a90 arch/x86/kvm/vmx.c:8278
> vmx_set_nested_state+0xc3a/0x1530 arch/x86/kvm/vmx.c:14045
> kvm_arch_vcpu_ioctl+0x4fc9/0x73a0 arch/x86/kvm/x86.c:4057
> kvm_vcpu_ioctl+0xca3/0x1f90 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2742
> do_vfs_ioctl+0xf77/0x2d30 fs/ioctl.c:46
> ksys_ioctl fs/ioctl.c:702 [inline]
> __do_sys_ioctl fs/ioctl.c:709 [inline]
> __se_sys_ioctl+0x1da/0x270 fs/ioctl.c:707
> __x64_sys_ioctl+0x4a/0x70 fs/ioctl.c:707
> do_syscall_64+0xcf/0x110 arch/x86/entry/common.c:291
> entry_SYSCALL_64_after_hwframe+0x63/0xe7
>
> Bytes 1000-4095 of 4096 are uninitialized
> Memory access of size 4096 starts at ffff8801b5157000
> ==================================================================
This appears to be a real bug in KVM.
Please see a simplified reproducer attached.
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at [email protected].
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
> syzbot.
> syzbot can test patches for this bug, for details see:
> https://goo.gl/tpsmEJ#testing-patches
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/0000000000005de8da057a092ba2%40google.com.
> For more options, visit https://groups.google.com/d/optout.
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
On 07/11/2018 13:10, Alexander Potapenko wrote:
> This appears to be a real bug in KVM.
> Please see a simplified reproducer attached.
Thanks, I agree it's a reael bug. The basic issue is that the
kvm_state->size member is too small (1040) in the KVM_SET_NESTED_STATE
ioctl, aka 0x4080aebf.
One way to fix it would be to just change kmalloc to kzalloc when
allocating cached_vmcs12 and cached_shadow_vmcs12, but really the ioctl
is wrong and should be rejected. And the case where a shadow VMCS has
to be loaded is even more wrong, and we have to fix it anyway, so I
don't really like the idea of papering over the bug in the allocation.
I'll test this patch and submit it formally:
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index c645f777b425..c546f0b1f3e0 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -14888,10 +14888,13 @@ static int vmx_set_nested_state(struct
kvm_vcpu *vcpu,
if (ret)
return ret;
- /* Empty 'VMXON' state is permitted */
- if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
+ /* Empty 'VMXON' state is permitted. A partial VMCS12 is not. */
+ if (kvm_state->size == sizeof(kvm_state))
return 0;
+ if (kvm_state->size < sizeof(kvm_state) + VMCS12_SIZE)
+ return -EINVAL;
+
if (kvm_state->vmx.vmcs_pa != -1ull) {
if (kvm_state->vmx.vmcs_pa == kvm_state->vmx.vmxon_pa ||
!page_address_valid(vcpu, kvm_state->vmx.vmcs_pa))
@@ -14917,6 +14920,7 @@ static int vmx_set_nested_state(struct kvm_vcpu
*vcpu,
}
vmcs12 = get_vmcs12(vcpu);
+ BUILD_BUG_ON(sizeof(*vmcs12) > VMCS12_SIZE);
if (copy_from_user(vmcs12, user_kvm_nested_state->data, sizeof(*vmcs12)))
return -EFAULT;
@@ -14932,7 +14936,7 @@ static int vmx_set_nested_state(struct kvm_vcpu
*vcpu,
if (nested_cpu_has_shadow_vmcs(vmcs12) &&
vmcs12->vmcs_link_pointer != -1ull) {
struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
- if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
+ if (kvm_state->size < sizeof(kvm_state) + 2 * VMCS12_SIZE)
return -EINVAL;
if (copy_from_user(shadow_vmcs12,
Paolo
> On 7 Nov 2018, at 14:10, Alexander Potapenko <[email protected]> wrote:
>
> On Wed, Nov 7, 2018 at 2:38 AM syzbot
> <[email protected]> wrote:
>>
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit: 88b95ef4c780 kmsan: use MSan assembly instrumentation
>> git tree: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_google_kmsan.git_master&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=NpjK8vgD4UmRGNWTNe6YFA-AJTZp9VlD0oMFvKFV25I&s=A5G9_UvV5TpBoJitbGWS2VXmfVMXFUgggq64QM-6nqc&e=
>> console output: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_log.txt-3Fx-3D12505e33400000&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=NpjK8vgD4UmRGNWTNe6YFA-AJTZp9VlD0oMFvKFV25I&s=ZGVw04dRMjYdKZf4amN1yl3IheljZZq6V9h3mO9U-vM&e=
>> kernel config: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_.config-3Fx-3D8df5fc509a1b351b&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=NpjK8vgD4UmRGNWTNe6YFA-AJTZp9VlD0oMFvKFV25I&s=OUIhmSMzBSMhswtebZqyLnc6SkAagVPBGr0EmCLL2Fg&e=
>> dashboard link: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_bug-3Fextid-3Dded1696f6b50b615b630&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=NpjK8vgD4UmRGNWTNe6YFA-AJTZp9VlD0oMFvKFV25I&s=jeCou6OWbHHIf190FBGsLr1wGsDvBWlpKnfNMmqGIqI&e=
>> compiler: clang version 8.0.0 (trunk 343298)
>> syz repro: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_repro.syz-3Fx-3D15ce62f5400000&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=NpjK8vgD4UmRGNWTNe6YFA-AJTZp9VlD0oMFvKFV25I&s=PVi1m-uNP3XRO4XbDZJicGiqXdVmOPFDxCP20jmXuAs&e=
>> C reproducer: https://urldefense.proofpoint.com/v2/url?u=https-3A__syzkaller.appspot.com_x_repro.c-3Fx-3D174efca3400000&d=DwIFaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=NpjK8vgD4UmRGNWTNe6YFA-AJTZp9VlD0oMFvKFV25I&s=XzvJtd3__2LEBvhAi4F6RTLbxV9mELkY07bMDSDLRMc&e=
>>
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: [email protected]
>>
>> ==================================================================
>> BUG: KMSAN: kernel-infoleak in __copy_to_user include/linux/uaccess.h:121
>> [inline]
>> BUG: KMSAN: kernel-infoleak in __kvm_write_guest_page
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:1849 [inline]
>> BUG: KMSAN: kernel-infoleak in kvm_vcpu_write_guest_page+0x39a/0x510
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:1870
>> CPU: 0 PID: 7918 Comm: syz-executor542 Not tainted 4.19.0+ #77
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
>> Google 01/01/2011
>> Call Trace:
>> __dump_stack lib/dump_stack.c:77 [inline]
>> dump_stack+0x32d/0x480 lib/dump_stack.c:113
>> kmsan_report+0x1a2/0x2e0 mm/kmsan/kmsan.c:911
>> kmsan_internal_check_memory+0x34c/0x430 mm/kmsan/kmsan.c:991
>> kmsan_copy_to_user+0x85/0xe0 mm/kmsan/kmsan_hooks.c:552
>> __copy_to_user include/linux/uaccess.h:121 [inline]
>> __kvm_write_guest_page arch/x86/kvm/../../../virt/kvm/kvm_main.c:1849
>> [inline]
>> kvm_vcpu_write_guest_page+0x39a/0x510
>> arch/x86/kvm/../../../virt/kvm/kvm_main.c:1870
>> nested_release_vmcs12 arch/x86/kvm/vmx.c:8441 [inline]
>> handle_vmptrld+0x2384/0x26b0 arch/x86/kvm/vmx.c:8907
>> vmx_handle_exit+0x1e81/0xbac0 arch/x86/kvm/vmx.c:10128
>> vcpu_enter_guest arch/x86/kvm/x86.c:7667 [inline]
>> vcpu_run arch/x86/kvm/x86.c:7730 [inline]
>> kvm_arch_vcpu_ioctl_run+0xac32/0x11d80 arch/x86/kvm/x86.c:7930
>> kvm_vcpu_ioctl+0xfb1/0x1f90 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2590
>> do_vfs_ioctl+0xf77/0x2d30 fs/ioctl.c:46
>> ksys_ioctl fs/ioctl.c:702 [inline]
>> __do_sys_ioctl fs/ioctl.c:709 [inline]
>> __se_sys_ioctl+0x1da/0x270 fs/ioctl.c:707
>> __x64_sys_ioctl+0x4a/0x70 fs/ioctl.c:707
>> do_syscall_64+0xcf/0x110 arch/x86/entry/common.c:291
>> entry_SYSCALL_64_after_hwframe+0x63/0xe7
>> RIP: 0033:0x44b6e9
>> Code: e8 dc e6 ff ff 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
>> 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
>> ff 0f 83 2b ff fb ff c3 66 2e 0f 1f 84 00 00 00 00
>> RSP: 002b:00007f096b292ce8 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
>> RAX: ffffffffffffffda RBX: 00000000006e3c48 RCX: 000000000044b6e9
>> RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000005
>> RBP: 00000000006e3c40 R08: 0000000000000000 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000206 R12: 00000000006e3c4c
>> R13: 00007ffd978aeb2f R14: 00007f096b2939c0 R15: 00000000006e3d4c
>>
>> Uninit was created at:
>> kmsan_save_stack_with_flags mm/kmsan/kmsan.c:252 [inline]
>> kmsan_internal_poison_shadow+0xc8/0x1e0 mm/kmsan/kmsan.c:177
>> kmsan_kmalloc+0x98/0x110 mm/kmsan/kmsan_hooks.c:104
>> __kmalloc+0x14c/0x4d0 mm/slub.c:3789
>> kmalloc include/linux/slab.h:518 [inline]
>> enter_vmx_operation+0x980/0x1a90 arch/x86/kvm/vmx.c:8278
>> vmx_set_nested_state+0xc3a/0x1530 arch/x86/kvm/vmx.c:14045
>> kvm_arch_vcpu_ioctl+0x4fc9/0x73a0 arch/x86/kvm/x86.c:4057
>> kvm_vcpu_ioctl+0xca3/0x1f90 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2742
>> do_vfs_ioctl+0xf77/0x2d30 fs/ioctl.c:46
>> ksys_ioctl fs/ioctl.c:702 [inline]
>> __do_sys_ioctl fs/ioctl.c:709 [inline]
>> __se_sys_ioctl+0x1da/0x270 fs/ioctl.c:707
>> __x64_sys_ioctl+0x4a/0x70 fs/ioctl.c:707
>> do_syscall_64+0xcf/0x110 arch/x86/entry/common.c:291
>> entry_SYSCALL_64_after_hwframe+0x63/0xe7
>>
>> Bytes 1000-4095 of 4096 are uninitialized
>> Memory access of size 4096 starts at ffff8801b5157000
>> ==================================================================
> This appears to be a real bug in KVM.
> Please see a simplified reproducer attached.
If I understand the above trace correctly:
1) When guest enters VMX operation, cached_vmcs12 is allocated in enter_vmx_operation() by kmalloc()
2) When guest will execute VMPTRLD, handle_vmptrld() will first release current vmcs12 by nested_release_vmcs12()
and only then copy newly loaded vmcs12 to cached_vmcs12 by memcpy().
3) The bug theoretically is that nested_release_vmcs12() copies entire cached_vmcs12 to guest memory which is
not initialised in case this is the very first VMPTRLD executed by guest.
But this case should not happen because nested_release_vmcs12() starts by checking if current_vmptr is equal to -1
and vmx_create_vcpu() set current_vmptr to -1ull.
However, one case where (3) could happen is by vmx_set_nested_state().
In case kvm_state->vmx.vmcs_pa is valid, set_current_vmptr() is called which sets current_vmptr.
However, it is possible that copy_from_user() which copies into cached_vmcs12 will fail.
Thus, leaving us with current_vmptr != -1 but with uninitialised cached_vmcs12.
Therefore, next VMPTRLD will indeed leak into guest the uninitialised content of cached_vmcs12.
What I described seems to be a bug to me (correct me if I’m wrong) but it doesn’t seem to be the
same issue described here as we can see that attached reproducer don’t use KVM_SET_NESTED_STATE ioctl.
-Liran
> On 7 Nov 2018, at 14:47, Paolo Bonzini <[email protected]> wrote:
>
> On 07/11/2018 13:10, Alexander Potapenko wrote:
>> This appears to be a real bug in KVM.
>> Please see a simplified reproducer attached.
>
> Thanks, I agree it's a reael bug. The basic issue is that the
> kvm_state->size member is too small (1040) in the KVM_SET_NESTED_STATE
> ioctl, aka 0x4080aebf.
>
> One way to fix it would be to just change kmalloc to kzalloc when
> allocating cached_vmcs12 and cached_shadow_vmcs12, but really the ioctl
> is wrong and should be rejected. And the case where a shadow VMCS has
> to be loaded is even more wrong, and we have to fix it anyway, so I
> don't really like the idea of papering over the bug in the allocation.
>
> I'll test this patch and submit it formally:
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index c645f777b425..c546f0b1f3e0 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -14888,10 +14888,13 @@ static int vmx_set_nested_state(struct
> kvm_vcpu *vcpu,
> if (ret)
> return ret;
>
> - /* Empty 'VMXON' state is permitted */
> - if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
> + /* Empty 'VMXON' state is permitted. A partial VMCS12 is not. */
> + if (kvm_state->size == sizeof(kvm_state))
> return 0;
>
> + if (kvm_state->size < sizeof(kvm_state) + VMCS12_SIZE)
> + return -EINVAL;
> +
I don’t think that this test is sufficient to fully resolve issue.
What if malicious userspace supplies valid size but pages containing nested_state->vmcs12 is unmapped?
This will result in vmx_set_nested_state() still calling set_current_vmptr() but failing on copy_from_user()
which still leaks cached_vmcs12 on next VMPTRLD of guest.
Therefore, I think that the correct patch should be to change vmx_set_nested_state() to
first gather all relevant information from userspace and validate it,
and only then start applying it to KVM’s internal vCPU state.
> if (kvm_state->vmx.vmcs_pa != -1ull) {
> if (kvm_state->vmx.vmcs_pa == kvm_state->vmx.vmxon_pa ||
> !page_address_valid(vcpu, kvm_state->vmx.vmcs_pa))
> @@ -14917,6 +14920,7 @@ static int vmx_set_nested_state(struct kvm_vcpu
> *vcpu,
> }
>
> vmcs12 = get_vmcs12(vcpu);
> + BUILD_BUG_ON(sizeof(*vmcs12) > VMCS12_SIZE);
Why put this BUILD_BUG_ON() specifically here?
There are many places which assumes cached_vmcs12 is of size VMCS12_SIZE.
(Such as nested_release_vmcs12() and handle_vmptrld()).
> if (copy_from_user(vmcs12, user_kvm_nested_state->data, sizeof(*vmcs12)))
> return -EFAULT;
>
> @@ -14932,7 +14936,7 @@ static int vmx_set_nested_state(struct kvm_vcpu
> *vcpu,
> if (nested_cpu_has_shadow_vmcs(vmcs12) &&
> vmcs12->vmcs_link_pointer != -1ull) {
> struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
> - if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
> + if (kvm_state->size < sizeof(kvm_state) + 2 * VMCS12_SIZE)
> return -EINVAL;
>
> if (copy_from_user(shadow_vmcs12,
>
> Paolo
-Liran
On 07/11/2018 13:58, Liran Alon wrote:
>
>
>> On 7 Nov 2018, at 14:47, Paolo Bonzini <[email protected]> wrote:
>>
>> On 07/11/2018 13:10, Alexander Potapenko wrote:
>>> This appears to be a real bug in KVM.
>>> Please see a simplified reproducer attached.
>>
>> Thanks, I agree it's a reael bug. The basic issue is that the
>> kvm_state->size member is too small (1040) in the KVM_SET_NESTED_STATE
>> ioctl, aka 0x4080aebf.
>>
>> One way to fix it would be to just change kmalloc to kzalloc when
>> allocating cached_vmcs12 and cached_shadow_vmcs12, but really the ioctl
>> is wrong and should be rejected. And the case where a shadow VMCS has
>> to be loaded is even more wrong, and we have to fix it anyway, so I
>> don't really like the idea of papering over the bug in the allocation.
>>
>> I'll test this patch and submit it formally:
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index c645f777b425..c546f0b1f3e0 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -14888,10 +14888,13 @@ static int vmx_set_nested_state(struct
>> kvm_vcpu *vcpu,
>> if (ret)
>> return ret;
>>
>> - /* Empty 'VMXON' state is permitted */
>> - if (kvm_state->size < sizeof(kvm_state) + sizeof(*vmcs12))
>> + /* Empty 'VMXON' state is permitted. A partial VMCS12 is not. */
>> + if (kvm_state->size == sizeof(kvm_state))
>> return 0;
>>
>> + if (kvm_state->size < sizeof(kvm_state) + VMCS12_SIZE)
>> + return -EINVAL;
>> +
>
> I don’t think that this test is sufficient to fully resolve issue.
> What if malicious userspace supplies valid size but pages containing nested_state->vmcs12 is unmapped?
> This will result in vmx_set_nested_state() still calling set_current_vmptr() but failing on copy_from_user()
> which still leaks cached_vmcs12 on next VMPTRLD of guest.
Makes sense; since SET_NESTED_STATE is not a fast path, we can just
memdup_user and pass a kernel pointer to vmx_set_nested_state.
> Therefore, I think that the correct patch should be to change vmx_set_nested_state() to
> first gather all relevant information from userspace and validate it,
> and only then start applying it to KVM’s internal vCPU state.
>
>> if (kvm_state->vmx.vmcs_pa != -1ull) {
>> if (kvm_state->vmx.vmcs_pa == kvm_state->vmx.vmxon_pa ||
>> !page_address_valid(vcpu, kvm_state->vmx.vmcs_pa))
>> @@ -14917,6 +14920,7 @@ static int vmx_set_nested_state(struct kvm_vcpu
>> *vcpu,
>> }
>>
>> vmcs12 = get_vmcs12(vcpu);
>> + BUILD_BUG_ON(sizeof(*vmcs12) > VMCS12_SIZE);
>
> Why put this BUILD_BUG_ON() specifically here?
> There are many places which assumes cached_vmcs12 is of size VMCS12_SIZE.
> (Such as nested_release_vmcs12() and handle_vmptrld()).
Unlike those places, here the copy has sizeof(*vmcs12) bytes and an
overflow would cause a userspace write to kernel memory. Though, that
means there is still a possibility of leaking kernel data when
nested_release_vmcs12 is called. So it also makes sense to use
VMCS12_SIZE for the memory copies, and kzalloc.
Thanks,
Paolo
>> if (copy_from_user(vmcs12, user_kvm_nested_state->data, sizeof(*vmcs12)))
>> return -EFAULT;
>>
>> @@ -14932,7 +14936,7 @@ static int vmx_set_nested_state(struct kvm_vcpu
>> *vcpu,
>> if (nested_cpu_has_shadow_vmcs(vmcs12) &&
>> vmcs12->vmcs_link_pointer != -1ull) {
>> struct vmcs12 *shadow_vmcs12 = get_shadow_vmcs12(vcpu);
>> - if (kvm_state->size < sizeof(kvm_state) + 2 * sizeof(*vmcs12))
>> + if (kvm_state->size < sizeof(kvm_state) + 2 * VMCS12_SIZE)
>> return -EINVAL;
>>
>> if (copy_from_user(shadow_vmcs12,
>>
>> Paolo
>
> -Liran
>
>
This changes the allocation of cached_vmcs12 to use kzalloc instead of
kmalloc. This removes the information leak found by Syzkaller (see
Reported-by) in this case and prevents similar leaks from happening
based on cached_vmcs12.
The email from Syszkaller led to a discussion about a patch in early
November on the KVM list (I've made this a reply to that thread), but
the current upstream kernel still has kmalloc instead of kzalloc for
cached_vmcs12 and cached_shadow_vmcs12. This RFC proposes changing to
kzalloc for defense in depth.
Tested: rebuilt but not tested, since this is an RFC
Reported-by: [email protected]
Signed-off-by: Tom Roeder <[email protected]>
---
arch/x86/kvm/vmx/nested.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 2616bd2c7f2c7..ad46667042c7a 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -4140,11 +4140,11 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)
if (r < 0)
goto out_vmcs02;
- vmx->nested.cached_vmcs12 = kmalloc(VMCS12_SIZE, GFP_KERNEL);
+ vmx->nested.cached_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL);
if (!vmx->nested.cached_vmcs12)
goto out_cached_vmcs12;
- vmx->nested.cached_shadow_vmcs12 = kmalloc(VMCS12_SIZE, GFP_KERNEL);
+ vmx->nested.cached_shadow_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL);
if (!vmx->nested.cached_shadow_vmcs12)
goto out_cached_shadow_vmcs12;
--
2.20.1.97.g81188d93c3-goog
On Mon, Jan 14, 2019 at 3:48 PM Tom Roeder <[email protected]> wrote:
>
> This changes the allocation of cached_vmcs12 to use kzalloc instead of
> kmalloc. This removes the information leak found by Syzkaller (see
> Reported-by) in this case and prevents similar leaks from happening
> based on cached_vmcs12.
>
> The email from Syszkaller led to a discussion about a patch in early
> November on the KVM list (I've made this a reply to that thread), but
> the current upstream kernel still has kmalloc instead of kzalloc for
> cached_vmcs12 and cached_shadow_vmcs12. This RFC proposes changing to
> kzalloc for defense in depth.
>
> Tested: rebuilt but not tested, since this is an RFC
>
> Reported-by: [email protected]
> Signed-off-by: Tom Roeder <[email protected]>
Reviewed-by: Jim Mattson <[email protected]>
On Mon, Jan 14, 2019 at 03:47:28PM -0800, Tom Roeder wrote:
> This changes the allocation of cached_vmcs12 to use kzalloc instead of
> kmalloc. This removes the information leak found by Syzkaller (see
> Reported-by) in this case and prevents similar leaks from happening
> based on cached_vmcs12.
Is the leak specific to vmx_set_nested_state(), e.g. can we zero out
the memory if copy_from_user() fails instead of taking the hit on every
allocation?
> The email from Syszkaller led to a discussion about a patch in early
> November on the KVM list (I've made this a reply to that thread), but
> the current upstream kernel still has kmalloc instead of kzalloc for
> cached_vmcs12 and cached_shadow_vmcs12. This RFC proposes changing to
> kzalloc for defense in depth.
>
> Tested: rebuilt but not tested, since this is an RFC
>
> Reported-by: [email protected]
> Signed-off-by: Tom Roeder <[email protected]>
> ---
> arch/x86/kvm/vmx/nested.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 2616bd2c7f2c7..ad46667042c7a 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4140,11 +4140,11 @@ static int enter_vmx_operation(struct kvm_vcpu *vcpu)
> if (r < 0)
> goto out_vmcs02;
>
> - vmx->nested.cached_vmcs12 = kmalloc(VMCS12_SIZE, GFP_KERNEL);
> + vmx->nested.cached_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL);
> if (!vmx->nested.cached_vmcs12)
> goto out_cached_vmcs12;
Obviously not your code, but why do we allocate VMCS12_SIZE instead of
sizeof(struct vmcs12)? I get why we require userspace to reserve the
full 4k, but I don't understand why KVM needs to allocate the reserved
bytes internally.
> - vmx->nested.cached_shadow_vmcs12 = kmalloc(VMCS12_SIZE, GFP_KERNEL);
> + vmx->nested.cached_shadow_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL);
> if (!vmx->nested.cached_shadow_vmcs12)
> goto out_cached_shadow_vmcs12;
>
> --
> 2.20.1.97.g81188d93c3-goog
>
On 15/01/19 03:43, Sean Christopherson wrote:
>> - vmx->nested.cached_vmcs12 = kmalloc(VMCS12_SIZE, GFP_KERNEL);
>> + vmx->nested.cached_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL);
>> if (!vmx->nested.cached_vmcs12)
>> goto out_cached_vmcs12;
> Obviously not your code, but why do we allocate VMCS12_SIZE instead of
> sizeof(struct vmcs12)? I get why we require userspace to reserve the
> full 4k, but I don't understand why KVM needs to allocate the reserved
> bytes internally.
It's just cleaner and shorter code to copy everything in and out,
instead of having to explicitly zero the slack.
Paolo
On Mon, Jan 14, 2019 at 06:43:04PM -0800, Sean Christopherson wrote:
> On Mon, Jan 14, 2019 at 03:47:28PM -0800, Tom Roeder wrote:
> > This changes the allocation of cached_vmcs12 to use kzalloc instead of
> > kmalloc. This removes the information leak found by Syzkaller (see
> > Reported-by) in this case and prevents similar leaks from happening
> > based on cached_vmcs12.
>
> Is the leak specific to vmx_set_nested_state(), e.g. can we zero out
> the memory if copy_from_user() fails instead of taking the hit on every
> allocation?
I don't know if the leak is specific to vmx_set_nested_state.
This question (and the rest of the thread from November) goes to the
heart of what I wanted to get feedback about; hence the "RFC" part of
the subject line.
I'm new to the kernel, and I don't know all the idioms and expectations,
so the follow analysis is an outsider's view of the issue at hand.
What I see in this case is a field that is intended to be copied to the
guest and is allocated initially with data from the kernel. I'm sure we
could figure out all the current paths and error cases that we need to
handle to make sure that this data never leaks. Future reviewers then
also need to make sure that changes to the nested VMX code don't leak
data from this field.
Why not instead make sure that there isn't any data to leak in the first
place?
I understand that there's a cost to kzalloc vs. kmalloc, but I don't
know what it is in practice; slab.c shows that the extra code for the
__GFP_ZERO flag is a memset of 0 over the allocated memory. The
allocation looks very infrequent for the two lines in this patch, since
they occur in enter_vmx_operation. That sounds to me like the allocation
only happens when the guest enables nested virtualization.
Given the frequency of allocation and the relative security benefit of
not having to worry about leaking the data, I'd advocate for changing it
here.
On Tue, Jan 15, 2019 at 11:15:51AM +0100, Paolo Bonzini wrote:
> On 15/01/19 03:43, Sean Christopherson wrote:
> >> - vmx->nested.cached_vmcs12 = kmalloc(VMCS12_SIZE, GFP_KERNEL);
> >> + vmx->nested.cached_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL);
> >> if (!vmx->nested.cached_vmcs12)
> >> goto out_cached_vmcs12;
> > Obviously not your code, but why do we allocate VMCS12_SIZE instead of
> > sizeof(struct vmcs12)? I get why we require userspace to reserve the
> > full 4k, but I don't understand why KVM needs to allocate the reserved
> > bytes internally.
>
> It's just cleaner and shorter code to copy everything in and out,
> instead of having to explicitly zero the slack.
Could you please clarify? I don't see code that copies everything in and
out, but it depends on what you mean by "everything". In the context of
this email exchange, I assumed that "everything" was "all 4k
(VMCS12_SIZE)".
But it looks to me like the code doesn't copy 4k in and out, but rather
only ever copies sizeof(struct vmcs12) in and out. The copy_from_user
and copy_to_user cases in nested.c use sizeof(*vmcs12), which is
sizeof(struct vmcs12).
So maybe can switch to allocating sizeof(struct vmcs12). Is this
correct, or is there some other reason to allocate the larger size?
On Tue, Jan 15, 2019 at 09:51:11AM -0800, Tom Roeder wrote:
> On Mon, Jan 14, 2019 at 06:43:04PM -0800, Sean Christopherson wrote:
> > On Mon, Jan 14, 2019 at 03:47:28PM -0800, Tom Roeder wrote:
> > > This changes the allocation of cached_vmcs12 to use kzalloc instead of
> > > kmalloc. This removes the information leak found by Syzkaller (see
> > > Reported-by) in this case and prevents similar leaks from happening
> > > based on cached_vmcs12.
> >
> > Is the leak specific to vmx_set_nested_state(), e.g. can we zero out
> > the memory if copy_from_user() fails instead of taking the hit on every
> > allocation?
>
> I don't know if the leak is specific to vmx_set_nested_state.
I've looked at the code more now, and it looks to me like there might be
other cases where the memory could leak. But I don't know enough of the
flows to be sure. The enter_vmx_operation function is called in
handle_vmon, and no data is copied from the guest immediately after
that. So, it depends on what happens after VMXON.
Even in vmx_set_nested_state, there are about 30 lines of code in
between enter_vmx_operation and copy_from_user, and there are a couple
of cases that cause vmx_set_nested_state to return with an error. So if
we want to fix this by handling all the error paths, I think it might be
cleanest to convert vmx_set_nested_state to use goto error handling,
since that would allow us to clear the allocated memory in one place.
What do you think?
On 23/01/19 19:25, Tom Roeder wrote:
> On Tue, Jan 15, 2019 at 11:15:51AM +0100, Paolo Bonzini wrote:
>> On 15/01/19 03:43, Sean Christopherson wrote:
>>>> - vmx->nested.cached_vmcs12 = kmalloc(VMCS12_SIZE, GFP_KERNEL);
>>>> + vmx->nested.cached_vmcs12 = kzalloc(VMCS12_SIZE, GFP_KERNEL);
>>>> if (!vmx->nested.cached_vmcs12)
>>>> goto out_cached_vmcs12;
>>> Obviously not your code, but why do we allocate VMCS12_SIZE instead of
>>> sizeof(struct vmcs12)? I get why we require userspace to reserve the
>>> full 4k, but I don't understand why KVM needs to allocate the reserved
>>> bytes internally.
>>
>> It's just cleaner and shorter code to copy everything in and out,
>> instead of having to explicitly zero the slack.
>
> Could you please clarify? I don't see code that copies everything in and
> out, but it depends on what you mean by "everything". In the context of
> this email exchange, I assumed that "everything" was "all 4k
> (VMCS12_SIZE)".
I was thinking of vmx_get_nested_state, but actually it only copies
sizeof(*vmcs12). However, that is the place where we should copy 4k out
of it, including the zeroes. Otherwise, our userspace clients (which
doesn't know sizeof(*vmcs12) could leak uninitialized data of their own.
Paolo
> But it looks to me like the code doesn't copy 4k in and out, but rather
> only ever copies sizeof(struct vmcs12) in and out. The copy_from_user
> and copy_to_user cases in nested.c use sizeof(*vmcs12), which is
> sizeof(struct vmcs12).
>
> So maybe can switch to allocating sizeof(struct vmcs12). Is this
> correct, or is there some other reason to allocate the larger size?
>
On 23/01/19 19:33, Tom Roeder wrote:
>
> Even in vmx_set_nested_state, there are about 30 lines of code in
> between enter_vmx_operation and copy_from_user, and there are a couple
> of cases that cause vmx_set_nested_state to return with an error. So if
> we want to fix this by handling all the error paths, I think it might be
> cleanest to convert vmx_set_nested_state to use goto error handling,
> since that would allow us to clear the allocated memory in one place.
I prefer to have kzalloc, and change vmx_get_nested_state to copy the
whole page including the trailing zeroes.
Paolo
On Thu, Jan 24, 2019 at 02:18:26AM +0100, Paolo Bonzini wrote:
> On 23/01/19 19:33, Tom Roeder wrote:
> >
> > Even in vmx_set_nested_state, there are about 30 lines of code in
> > between enter_vmx_operation and copy_from_user, and there are a couple
> > of cases that cause vmx_set_nested_state to return with an error. So if
> > we want to fix this by handling all the error paths, I think it might be
> > cleanest to convert vmx_set_nested_state to use goto error handling,
> > since that would allow us to clear the allocated memory in one place.
>
> I prefer to have kzalloc, and change vmx_get_nested_state to copy the
> whole page including the trailing zeroes.
OK, sending as a v2 patch in a new thread.