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

From: Paolo Bonzini
Date: Wed Sep 11 2019 - 12:25:48 EST


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