[PATCH] KVM: X86: Fix async pf caused null-ptr-deref

[PATCH] KVM: X86: Fix async pf caused null-ptr-deref

[Wanpeng Li]

From: Wanpeng Li <wanpengli@tencent.com>

Syzbot reported that:

  CPU: 1 PID: 6780 Comm: syz-executor153 Not tainted 5.7.0-syzkaller #0
  Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
  RIP: 0010:__apic_accept_irq+0x46/0xb80
  Call Trace:
   kvm_arch_async_page_present+0x7de/0x9e0
   kvm_check_async_pf_completion+0x18d/0x400
   kvm_arch_vcpu_ioctl_run+0x18bf/0x69f0
   kvm_vcpu_ioctl+0x46a/0xe20
   ksys_ioctl+0x11a/0x180
   __x64_sys_ioctl+0x6f/0xb0
   do_syscall_64+0xf6/0x7d0
   entry_SYSCALL_64_after_hwframe+0x49/0xb3
 
The testcase enables APF mechanism in MSR_KVM_ASYNC_PF_EN with ASYNC_PF_INT 
enabled w/o setting MSR_KVM_ASYNC_PF_INT before, what's worse, interrupt 
based APF 'page ready' event delivery depends on in kernel lapic, however, 
we didn't bail out when lapic is not in kernel during guest setting 
MSR_KVM_ASYNC_PF_EN which causes the null-ptr-deref in host later. 
This patch fixes it.

Reported-by: syzbot+1bf777dfdde86d64b89b@syzkaller.appspotmail.com
Fixes: 2635b5c4a0 (KVM: x86: interrupt based APF 'page ready' event delivery)
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/x86.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 00c88c2..1c0b4f5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2693,6 +2693,9 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
 	if (data & 0x30)
 		return 1;
 
+	if (!lapic_in_kernel(vcpu))
+		return 1;
+
 	vcpu->arch.apf.msr_en_val = data;
 
 	if (!kvm_pv_async_pf_enabled(vcpu)) {
-- 
2.7.4

Re: [PATCH] KVM: X86: Fix async pf caused null-ptr-deref

[Vitaly Kuznetsov]

Wanpeng Li <kernellwp@gmail.com> writes:

> From: Wanpeng Li <wanpengli@tencent.com>
>
> Syzbot reported that:
>
>   CPU: 1 PID: 6780 Comm: syz-executor153 Not tainted 5.7.0-syzkaller #0
>   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>   RIP: 0010:__apic_accept_irq+0x46/0xb80
>   Call Trace:
>    kvm_arch_async_page_present+0x7de/0x9e0
>    kvm_check_async_pf_completion+0x18d/0x400
>    kvm_arch_vcpu_ioctl_run+0x18bf/0x69f0
>    kvm_vcpu_ioctl+0x46a/0xe20
>    ksys_ioctl+0x11a/0x180
>    __x64_sys_ioctl+0x6f/0xb0
>    do_syscall_64+0xf6/0x7d0
>    entry_SYSCALL_64_after_hwframe+0x49/0xb3
>  
> The testcase enables APF mechanism in MSR_KVM_ASYNC_PF_EN with ASYNC_PF_INT 
> enabled w/o setting MSR_KVM_ASYNC_PF_INT before, what's worse, interrupt 
> based APF 'page ready' event delivery depends on in kernel lapic, however, 
> we didn't bail out when lapic is not in kernel during guest setting 
> MSR_KVM_ASYNC_PF_EN which causes the null-ptr-deref in host later. 
> This patch fixes it.
>
> Reported-by: syzbot+1bf777dfdde86d64b89b@syzkaller.appspotmail.com
> Fixes: 2635b5c4a0 (KVM: x86: interrupt based APF 'page ready' event delivery)

Thanks!

> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  arch/x86/kvm/x86.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 00c88c2..1c0b4f5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2693,6 +2693,9 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
>  	if (data & 0x30)
>  		return 1;
>  
> +	if (!lapic_in_kernel(vcpu))
> +		return 1;
> +

I'm not sure how much we care about !lapic_in_kernel() case but this
change should be accompanied with userspace changes to not expose
KVM_FEATURE_ASYNC_PF_INT or how would the guest know that writing a
legitimate value will result in #GP?

Alternatively, we may just return '0' here: guest will be able to check
what's in the MSR to see if the feature was enabled. Normally, guests
shouldn't care about this but maybe there are cases when they do?

>  	vcpu->arch.apf.msr_en_val = data;
>  
>  	if (!kvm_pv_async_pf_enabled(vcpu)) {

-- 
Vitaly

Re: [PATCH] KVM: X86: Fix async pf caused null-ptr-deref

[Paolo Bonzini]

On 29/06/20 15:46, Vitaly Kuznetsov wrote:
>> +	if (!lapic_in_kernel(vcpu))
>> +		return 1;
>> +
> I'm not sure how much we care about !lapic_in_kernel() case but this
> change should be accompanied with userspace changes to not expose
> KVM_FEATURE_ASYNC_PF_INT or how would the guest know that writing a
> legitimate value will result in #GP?

Almost any pv feature is broken with QEMU if kernel_irqchip=off.  I
wouldn't bother and I am seriously thinking of dropping all support for
that, including:

- just injecting #UD for MOV from/to CR8 unless lapic_in_kernel()

- make KVM_INTERRUPT fail unless irqchip_in_kernel(), so that
KVM_INTERRUPT is only used to inject EXTINT with kernel_irqchip=split

Paolo

> Alternatively, we may just return '0' here: guest will be able to check
> what's in the MSR to see if the feature was enabled. Normally, guests
> shouldn't care about this but maybe there are cases when they do?
> 

Re: [PATCH] KVM: X86: Fix async pf caused null-ptr-deref

[Paolo Bonzini]

On 29/06/20 12:26, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> Syzbot reported that:
> 
>   CPU: 1 PID: 6780 Comm: syz-executor153 Not tainted 5.7.0-syzkaller #0
>   Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>   RIP: 0010:__apic_accept_irq+0x46/0xb80
>   Call Trace:
>    kvm_arch_async_page_present+0x7de/0x9e0
>    kvm_check_async_pf_completion+0x18d/0x400
>    kvm_arch_vcpu_ioctl_run+0x18bf/0x69f0
>    kvm_vcpu_ioctl+0x46a/0xe20
>    ksys_ioctl+0x11a/0x180
>    __x64_sys_ioctl+0x6f/0xb0
>    do_syscall_64+0xf6/0x7d0
>    entry_SYSCALL_64_after_hwframe+0x49/0xb3
>  
> The testcase enables APF mechanism in MSR_KVM_ASYNC_PF_EN with ASYNC_PF_INT 
> enabled w/o setting MSR_KVM_ASYNC_PF_INT before, what's worse, interrupt 
> based APF 'page ready' event delivery depends on in kernel lapic, however, 
> we didn't bail out when lapic is not in kernel during guest setting 
> MSR_KVM_ASYNC_PF_EN which causes the null-ptr-deref in host later. 
> This patch fixes it.
> 
> Reported-by: syzbot+1bf777dfdde86d64b89b@syzkaller.appspotmail.com
> Fixes: 2635b5c4a0 (KVM: x86: interrupt based APF 'page ready' event delivery)
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  arch/x86/kvm/x86.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 00c88c2..1c0b4f5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2693,6 +2693,9 @@ static int kvm_pv_enable_async_pf(struct kvm_vcpu *vcpu, u64 data)
>  	if (data & 0x30)
>  		return 1;
>  
> +	if (!lapic_in_kernel(vcpu))
> +		return 1;
> +
>  	vcpu->arch.apf.msr_en_val = data;
>  
>  	if (!kvm_pv_async_pf_enabled(vcpu)) {
> 

Queued, thanks.

Paolo

Re: [PATCH] KVM: X86: Fix async pf caused null-ptr-deref

[Sean Christopherson]

On Mon, Jun 29, 2020 at 03:59:25PM +0200, Paolo Bonzini wrote:
> On 29/06/20 15:46, Vitaly Kuznetsov wrote:
> >> +	if (!lapic_in_kernel(vcpu))
> >> +		return 1;
> >> +
> > I'm not sure how much we care about !lapic_in_kernel() case but this
> > change should be accompanied with userspace changes to not expose
> > KVM_FEATURE_ASYNC_PF_INT or how would the guest know that writing a
> > legitimate value will result in #GP?
> 
> Almost any pv feature is broken with QEMU if kernel_irqchip=off.  I
> wouldn't bother and I am seriously thinking of dropping all support for
> that, including:

Heh, based on my limited testing, that could be "Almost everything is
broken with Qemu if kernel_irqchip=off".

> - just injecting #UD for MOV from/to CR8 unless lapic_in_kernel()
> 
> - make KVM_INTERRUPT fail unless irqchip_in_kernel(), so that
> KVM_INTERRUPT is only used to inject EXTINT with kernel_irqchip=split
> 
> Paolo
> 
> > Alternatively, we may just return '0' here: guest will be able to check
> > what's in the MSR to see if the feature was enabled. Normally, guests
> > shouldn't care about this but maybe there are cases when they do?
> > 
> 

Re: [PATCH] KVM: X86: Fix async pf caused null-ptr-deref

[Wanpeng Li]

On Mon, 29 Jun 2020 at 23:42, Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Mon, Jun 29, 2020 at 03:59:25PM +0200, Paolo Bonzini wrote:
> > On 29/06/20 15:46, Vitaly Kuznetsov wrote:
> > >> +  if (!lapic_in_kernel(vcpu))
> > >> +          return 1;
> > >> +
> > > I'm not sure how much we care about !lapic_in_kernel() case but this
> > > change should be accompanied with userspace changes to not expose
> > > KVM_FEATURE_ASYNC_PF_INT or how would the guest know that writing a
> > > legitimate value will result in #GP?
> >
> > Almost any pv feature is broken with QEMU if kernel_irqchip=off.  I
> > wouldn't bother and I am seriously thinking of dropping all support for
> > that, including:
>
> Heh, based on my limited testing, that could be "Almost everything is
> broken with Qemu if kernel_irqchip=off".

It is broken for several years. :)