On 06/09/19 03:54, Wanpeng Li wrote: > On Thu, 5 Sep 2019 at 21:16, Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote: >> >> Wanpeng Li <kernellwp@xxxxxxxxx> writes: >> >>> From: Wanpeng Li <wanpengli@xxxxxxxxxxx> >>> >>> Reported by syzkaller: >>> >>> kasan: GPF could be caused by NULL-ptr deref or user memory access >>> general protection fault: 0000 [#1] PREEMPT SMP KASAN >>> RIP: 0010:__apic_accept_irq+0x46/0x740 arch/x86/kvm/lapic.c:1029 >>> Call Trace: >>> kvm_apic_set_irq+0xb4/0x140 arch/x86/kvm/lapic.c:558 >>> stimer_notify_direct arch/x86/kvm/hyperv.c:648 [inline] >>> stimer_expiration arch/x86/kvm/hyperv.c:659 [inline] >>> kvm_hv_process_stimers+0x594/0x1650 arch/x86/kvm/hyperv.c:686 >>> vcpu_enter_guest+0x2b2a/0x54b0 arch/x86/kvm/x86.c:7896 >>> vcpu_run+0x393/0xd40 arch/x86/kvm/x86.c:8152 >>> kvm_arch_vcpu_ioctl_run+0x636/0x900 arch/x86/kvm/x86.c:8360 >>> kvm_vcpu_ioctl+0x6cf/0xaf0 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2765 >>> >>> The testcase programs HV_X64_MSR_STIMERn_CONFIG/HV_X64_MSR_STIMERn_COUNT, >>> in addition, there is no lapic in the kernel, the counters value are small >>> enough in order that kvm_hv_process_stimers() inject this already-expired >>> timer interrupt into the guest through lapic in the kernel which triggers >>> the NULL deferencing. This patch fixes it by checking lapic_in_kernel, >>> discarding the inject if it is 0. >>> >>> Reported-by: syzbot+dff25ee91f0c7d5c1695@xxxxxxxxxxxxxxxxxxxxxxxxx >>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx> >>> Cc: Radim Krčmář <rkrcmar@xxxxxxxxxx> >>> Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> >>> Signed-off-by: Wanpeng Li <wanpengli@xxxxxxxxxxx> >>> --- >>> arch/x86/kvm/hyperv.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c >>> index c10a8b1..461fcc5 100644 >>> --- a/arch/x86/kvm/hyperv.c >>> +++ b/arch/x86/kvm/hyperv.c >>> @@ -645,7 +645,9 @@ static int stimer_notify_direct(struct kvm_vcpu_hv_stimer *stimer) >>> .vector = stimer->config.apic_vector >>> }; >>> >>> - return !kvm_apic_set_irq(vcpu, &irq, NULL); >>> + if (lapic_in_kernel(vcpu)) >>> + return !kvm_apic_set_irq(vcpu, &irq, NULL); >>> + return 0; >>> } >>> >>> static void stimer_expiration(struct kvm_vcpu_hv_stimer *stimer) >> >> Hm, but this basically means direct mode synthetic timers won't work >> when LAPIC is not in kernel but the feature will still be advertised to >> the guest, not good. Shall we stop advertizing it? Something like >> (completely untested): >> >> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c >> index 3f5ad84853fb..1dfa594eaab6 100644 >> --- a/arch/x86/kvm/hyperv.c >> +++ b/arch/x86/kvm/hyperv.c >> @@ -1856,7 +1856,13 @@ int kvm_vcpu_ioctl_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid, >> >> ent->edx |= HV_FEATURE_FREQUENCY_MSRS_AVAILABLE; >> ent->edx |= HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE; >> - ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE; >> + >> + /* >> + * Direct Synthetic timers only make sense with in-kernel >> + * LAPIC >> + */ >> + if (lapic_in_kernel(vcpu)) >> + ent->edx |= HV_STIMER_DIRECT_MODE_AVAILABLE; >> >> break; > > Thanks, I fold this into v2, syzkaller even didn't check the cpuid, so > I still keep the discard inject part. Can you also include a beautified testcase or at least a link to the syzkaller source? Thanks, Paolo
