Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] KVM: LAPIC: Fix SynIC Timers inject timer interrupt w/o
 LAPIC present
To:     Wanpeng Li <kernellwp@gmail.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>
Cc:     LKML <linux-kernel@vger.kernel.org>, kvm <kvm@vger.kernel.org>,
        =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>
References: <1567680270-14022-1-git-send-email-wanpengli@tencent.com>
 <87ftlakhn6.fsf@vitty.brq.redhat.com>
 <CANRm+CyPxb+ZY2cTdbLL_LBKMJSOaMqnPGKc_ATc6-TMHW-rJw@mail.gmail.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Openpgp: preference=signencrypt
Message-ID: <23b9c284-9317-2c9a-ed3d-6bdf9ac0dea5@redhat.com>
Date:   Wed, 11 Sep 2019 18:25:40 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <CANRm+CyPxb+ZY2cTdbLL_LBKMJSOaMqnPGKc_ATc6-TMHW-rJw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 06/09/19 03:54, Wanpeng Li wrote:
> On Thu, 5 Sep 2019 at 21:16, Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> Wanpeng Li <kernellwp@gmail.com> writes:
>>
>>> From: Wanpeng Li <wanpengli@tencent.com>
>>>
>>> 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@syzkaller.appspotmail.com
>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>> Cc: Radim Krčmář <rkrcmar@redhat.com>
>>> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
>>> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
>>> ---
>>>  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