| Message ID | 1513138624-24753-1-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 13/12/2017 05:17, Lan Tianyu wrote: > Reported by syzkaller: > WARNING: CPU: 0 PID: 27962 at arch/x86/kvm/emulate.c:5631 x86_emulate_insn+0x557/0x15f0 [kvm] > Modules linked in: kvm_intel kvm [last unloaded: kvm] > CPU: 0 PID: 27962 Comm: syz-executor Tainted: G B W 4.15.0-rc2-next-20171208+ #32 > Hardware name: Intel Corporation S1200SP/S1200SP, BIOS S1200SP.86B.01.03.0006.040720161253 04/07/2016 > RIP: 0010:x86_emulate_insn+0x557/0x15f0 [kvm] > RSP: 0018:ffff8807234476d0 EFLAGS: 00010282 > RAX: 0000000000000000 RBX: ffff88072d0237a0 RCX: ffffffffa0065c4d > RDX: 1ffff100e5a046f9 RSI: 0000000000000003 RDI: ffff88072d0237c8 > RBP: ffff880723447728 R08: ffff88072d020000 R09: ffffffffa008d240 > R10: 0000000000000002 R11: ffffed00e7d87db3 R12: ffff88072d0237c8 > R13: ffff88072d023870 R14: ffff88072d0238c2 R15: ffffffffa008d080 > FS: 00007f8a68666700(0000) GS:ffff880802200000(0000) knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 000000002009506c CR3: 000000071fec4005 CR4: 00000000003626f0 > Call Trace: > x86_emulate_instruction+0x3bc/0xb70 [kvm] > ? reexecute_instruction.part.162+0x130/0x130 [kvm] > vmx_handle_exit+0x46d/0x14f0 [kvm_intel] > ? trace_event_raw_event_kvm_entry+0xe7/0x150 [kvm] > ? handle_vmfunc+0x2f0/0x2f0 [kvm_intel] > ? wait_lapic_expire+0x25/0x270 [kvm] > vcpu_enter_guest+0x720/0x1ef0 [kvm] > ... > > When cs.l is set, vcpu should run in the 64 bit paging mode. > Current kvm set_sregs function doesn't have such check when userspace > inputs sreg values. This will lead unexpected behavior. This patch > is to add such check. > > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > 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: Jim Mattson <jmattson@google.com> > Signed-off-by: Tianyu Lan <tianyu.lan@intel.com> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > arch/x86/kvm/x86.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 1e1e617..07d6d6c 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -7485,6 +7485,18 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index, > } > EXPORT_SYMBOL_GPL(kvm_task_switch); > > +int kvm_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) > +{ > + /* When cs.l is set, vcpu should run in 64-bit mode */ > + if (sregs->cs.l) > + if (!((sregs->cr0 & X86_CR0_PG_BIT) && > + (sregs->cr4 & X86_CR4_PAE_BIT) && > + (sregs->efer & EFER_LME))) > + return -EINVAL; > + > + return 0; > +} > + > int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > struct kvm_sregs *sregs) > { > @@ -7497,6 +7509,9 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, > (sregs->cr4 & X86_CR4_OSXSAVE)) > return -EINVAL; > > + if (kvm_valid_sregs(vcpu, sregs)) > + return -EINVAL; > + > apic_base_msr.data = sregs->apic_base; > apic_base_msr.host_initiated = true; > if (kvm_set_apic_base(vcpu, &apic_base_msr)) > Thanks, this is the right direction. However, there are many more checks that are missing and can trigger more unexpected behavior. In particular: - if EFER.LME=1 and CR0.PG=1, EFER.LMA and CR4.PAE must be 1 - otherwise, EFER.LMA and CS.L must be 0 Thanks, Paolo
On 2017年12月13日 20:20, Paolo Bonzini wrote: > On 13/12/2017 05:17, Lan Tianyu wrote: >> Reported by syzkaller: >> WARNING: CPU: 0 PID: 27962 at arch/x86/kvm/emulate.c:5631 x86_emulate_insn+0x557/0x15f0 [kvm] >> Modules linked in: kvm_intel kvm [last unloaded: kvm] >> CPU: 0 PID: 27962 Comm: syz-executor Tainted: G B W 4.15.0-rc2-next-20171208+ #32 >> Hardware name: Intel Corporation S1200SP/S1200SP, BIOS S1200SP.86B.01.03.0006.040720161253 04/07/2016 >> RIP: 0010:x86_emulate_insn+0x557/0x15f0 [kvm] >> RSP: 0018:ffff8807234476d0 EFLAGS: 00010282 >> RAX: 0000000000000000 RBX: ffff88072d0237a0 RCX: ffffffffa0065c4d >> RDX: 1ffff100e5a046f9 RSI: 0000000000000003 RDI: ffff88072d0237c8 >> RBP: ffff880723447728 R08: ffff88072d020000 R09: ffffffffa008d240 >> R10: 0000000000000002 R11: ffffed00e7d87db3 R12: ffff88072d0237c8 >> R13: ffff88072d023870 R14: ffff88072d0238c2 R15: ffffffffa008d080 >> FS: 00007f8a68666700(0000) GS:ffff880802200000(0000) knlGS:0000000000000000 >> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> CR2: 000000002009506c CR3: 000000071fec4005 CR4: 00000000003626f0 >> Call Trace: >> x86_emulate_instruction+0x3bc/0xb70 [kvm] >> ? reexecute_instruction.part.162+0x130/0x130 [kvm] >> vmx_handle_exit+0x46d/0x14f0 [kvm_intel] >> ? trace_event_raw_event_kvm_entry+0xe7/0x150 [kvm] >> ? handle_vmfunc+0x2f0/0x2f0 [kvm_intel] >> ? wait_lapic_expire+0x25/0x270 [kvm] >> vcpu_enter_guest+0x720/0x1ef0 [kvm] >> ... >> >> When cs.l is set, vcpu should run in the 64 bit paging mode. >> Current kvm set_sregs function doesn't have such check when userspace >> inputs sreg values. This will lead unexpected behavior. This patch >> is to add such check. >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> 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: Jim Mattson <jmattson@google.com> >> Signed-off-by: Tianyu Lan <tianyu.lan@intel.com> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> arch/x86/kvm/x86.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index 1e1e617..07d6d6c 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -7485,6 +7485,18 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index, >> } >> EXPORT_SYMBOL_GPL(kvm_task_switch); >> >> +int kvm_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) >> +{ >> + /* When cs.l is set, vcpu should run in 64-bit mode */ >> + if (sregs->cs.l) >> + if (!((sregs->cr0 & X86_CR0_PG_BIT) && >> + (sregs->cr4 & X86_CR4_PAE_BIT) && >> + (sregs->efer & EFER_LME))) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, >> struct kvm_sregs *sregs) >> { >> @@ -7497,6 +7509,9 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, >> (sregs->cr4 & X86_CR4_OSXSAVE)) >> return -EINVAL; >> >> + if (kvm_valid_sregs(vcpu, sregs)) >> + return -EINVAL; >> + >> apic_base_msr.data = sregs->apic_base; >> apic_base_msr.host_initiated = true; >> if (kvm_set_apic_base(vcpu, &apic_base_msr)) >> > > Thanks, this is the right direction. However, there are many more > checks that are missing and can trigger more unexpected behavior. Yes, we may add these checks step by step. > In > particular: > > - if EFER.LME=1 and CR0.PG=1, EFER.LMA and CR4.PAE must be 1 > > - otherwise, EFER.LMA and CS.L must be 0 I will add these in the new version. Thanks.
On 14/12/2017 07:36, Lan Tianyu wrote: >> Thanks, this is the right direction. However, there are many more >> checks that are missing and can trigger more unexpected behavior. > > Yes, we may add these checks step by step. I agree with not fixing everything at the same time; however, EFER.LMA/CS.L checks are more or less all in the same family so they should be done together. I'll wait for v2. Thanks, Paolo >> In particular: >> >> - if EFER.LME=1 and CR0.PG=1, EFER.LMA and CR4.PAE must be 1 >> >> - otherwise, EFER.LMA and CS.L must be 0 > > I will add these in the new version. Thanks.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 1e1e617..07d6d6c 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -7485,6 +7485,18 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index, } EXPORT_SYMBOL_GPL(kvm_task_switch); +int kvm_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) +{ + /* When cs.l is set, vcpu should run in 64-bit mode */ + if (sregs->cs.l) + if (!((sregs->cr0 & X86_CR0_PG_BIT) && + (sregs->cr4 & X86_CR4_PAE_BIT) && + (sregs->efer & EFER_LME))) + return -EINVAL; + + return 0; +} + int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) { @@ -7497,6 +7509,9 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu, (sregs->cr4 & X86_CR4_OSXSAVE)) return -EINVAL; + if (kvm_valid_sregs(vcpu, sregs)) + return -EINVAL; + apic_base_msr.data = sregs->apic_base; apic_base_msr.host_initiated = true; if (kvm_set_apic_base(vcpu, &apic_base_msr))
Reported by syzkaller: WARNING: CPU: 0 PID: 27962 at arch/x86/kvm/emulate.c:5631 x86_emulate_insn+0x557/0x15f0 [kvm] Modules linked in: kvm_intel kvm [last unloaded: kvm] CPU: 0 PID: 27962 Comm: syz-executor Tainted: G B W 4.15.0-rc2-next-20171208+ #32 Hardware name: Intel Corporation S1200SP/S1200SP, BIOS S1200SP.86B.01.03.0006.040720161253 04/07/2016 RIP: 0010:x86_emulate_insn+0x557/0x15f0 [kvm] RSP: 0018:ffff8807234476d0 EFLAGS: 00010282 RAX: 0000000000000000 RBX: ffff88072d0237a0 RCX: ffffffffa0065c4d RDX: 1ffff100e5a046f9 RSI: 0000000000000003 RDI: ffff88072d0237c8 RBP: ffff880723447728 R08: ffff88072d020000 R09: ffffffffa008d240 R10: 0000000000000002 R11: ffffed00e7d87db3 R12: ffff88072d0237c8 R13: ffff88072d023870 R14: ffff88072d0238c2 R15: ffffffffa008d080 FS: 00007f8a68666700(0000) GS:ffff880802200000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 000000002009506c CR3: 000000071fec4005 CR4: 00000000003626f0 Call Trace: x86_emulate_instruction+0x3bc/0xb70 [kvm] ? reexecute_instruction.part.162+0x130/0x130 [kvm] vmx_handle_exit+0x46d/0x14f0 [kvm_intel] ? trace_event_raw_event_kvm_entry+0xe7/0x150 [kvm] ? handle_vmfunc+0x2f0/0x2f0 [kvm_intel] ? wait_lapic_expire+0x25/0x270 [kvm] vcpu_enter_guest+0x720/0x1ef0 [kvm] ... When cs.l is set, vcpu should run in the 64 bit paging mode. Current kvm set_sregs function doesn't have such check when userspace inputs sreg values. This will lead unexpected behavior. This patch is to add such check. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> 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: Jim Mattson <jmattson@google.com> Signed-off-by: Tianyu Lan <tianyu.lan@intel.com> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- arch/x86/kvm/x86.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)