KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] KVM: LAPIC: Fix SynIC Timers inject timer interrupt w/o LAPIC present
@ 2019-09-05 10:44 Wanpeng Li
  2019-09-05 13:16 ` Vitaly Kuznetsov
  0 siblings, 1 reply; 4+ messages in thread
From: Wanpeng Li @ 2019-09-05 10:44 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Radim Krčmář, Vitaly Kuznetsov

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)
-- 
2.7.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: LAPIC: Fix SynIC Timers inject timer interrupt w/o LAPIC present
  2019-09-05 10:44 [PATCH] KVM: LAPIC: Fix SynIC Timers inject timer interrupt w/o LAPIC present Wanpeng Li
@ 2019-09-05 13:16 ` Vitaly Kuznetsov
  2019-09-06  1:54   ` Wanpeng Li
  0 siblings, 1 reply; 4+ messages in thread
From: Vitaly Kuznetsov @ 2019-09-05 13:16 UTC (permalink / raw)
  To: Wanpeng Li, linux-kernel, kvm; +Cc: Paolo Bonzini, Radim Krčmář

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;



-- 
Vitaly

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: LAPIC: Fix SynIC Timers inject timer interrupt w/o LAPIC present
  2019-09-05 13:16 ` Vitaly Kuznetsov
@ 2019-09-06  1:54   ` Wanpeng Li
  2019-09-11 16:25     ` Paolo Bonzini
  0 siblings, 1 reply; 4+ messages in thread
From: Wanpeng Li @ 2019-09-06  1:54 UTC (permalink / raw)
  To: Vitaly Kuznetsov; +Cc: LKML, kvm, Paolo Bonzini, Radim Krčmář

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.

                                                                   Wanpeng

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: LAPIC: Fix SynIC Timers inject timer interrupt w/o LAPIC present
  2019-09-06  1:54   ` Wanpeng Li
@ 2019-09-11 16:25     ` Paolo Bonzini
  0 siblings, 0 replies; 4+ messages in thread
From: Paolo Bonzini @ 2019-09-11 16:25 UTC (permalink / raw)
  To: Wanpeng Li, Vitaly Kuznetsov; +Cc: LKML, kvm, Radim Krčmář

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05 10:44 [PATCH] KVM: LAPIC: Fix SynIC Timers inject timer interrupt w/o LAPIC present Wanpeng Li
2019-09-05 13:16 ` Vitaly Kuznetsov
2019-09-06  1:54   ` Wanpeng Li
2019-09-11 16:25     ` Paolo Bonzini

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org kvm@archiver.kernel.org
	public-inbox-index kvm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox