Re: [PATCH] KVM: LAPIC: Fix SynIC Timers inject timer interrupt w/o LAPIC present

[Paolo Bonzini]

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