Re: [PATCH] KVM: ioapic: fix NULL deref ioapic->lock

[Paolo Bonzini]

On 01/01/2017 04:44, Wanpeng Li wrote:
> From: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> 
> This was reported by syzkaller:
> 
> BUG: unable to handle kernel NULL pointer dereference at 00000000000001b0
> IP: _raw_spin_lock+0xc/0x30
> PGD 3e28eb067
> PUD 3f0ac6067
> PMD 0
> Oops: 0002 [#1] SMP
> CPU: 0 PID: 2431 Comm: test Tainted: G           OE   4.10.0-rc1+ #3
> Call Trace:
>  ? kvm_ioapic_scan_entry+0x3e/0x110 [kvm]
>  kvm_arch_vcpu_ioctl_run+0x10a8/0x15f0 [kvm]
>  ? pick_next_task_fair+0xe1/0x4e0
>  ? kvm_arch_vcpu_load+0xea/0x260 [kvm]
>  kvm_vcpu_ioctl+0x33a/0x600 [kvm]
>  ? hrtimer_try_to_cancel+0x29/0x130
>  ? do_nanosleep+0x97/0xf0
>  do_vfs_ioctl+0xa1/0x5d0
>  ? __hrtimer_init+0x90/0x90
>  ? do_nanosleep+0x5b/0xf0
>  SyS_ioctl+0x79/0x90
>  do_syscall_64+0x6e/0x180
>  entry_SYSCALL64_slow_path+0x25/0x25
> RIP: _raw_spin_lock+0xc/0x30 RSP: ffffa43688973cc0
> 
> KVM will skip over create pic/ioapic if there is a created vCPU. However, 
> there is no guarantee whether ioapic is present when rescan ioapic which 
> results in NULL dereference ioapic->lock. This patch fix it by adding the 
> ioapic present check to ioapic scan.
> 
> Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> Signed-off-by: Wanpeng Li <wanpeng.li@xxxxxxxxxxx>
> ---
>  arch/x86/kvm/x86.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 51ccfe0..9ca175c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6556,7 +6556,8 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>  	else {
>  		if (vcpu->arch.apicv_active)
>  			kvm_x86_ops->sync_pir_to_irr(vcpu);
> -		kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
> +		if (ioapic_irqchip(vcpu->kvm))
> +			kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
>  	}
>  	bitmap_or((ulong *)eoi_exit_bitmap, vcpu->arch.ioapic_handled_vectors,
>  		  vcpu_to_synic(vcpu)->vec_bitmap, 256);

Commit message for fuzzing bugs have usually included a beautified
reproducer.  However, you are not even saying if it is a race, or it is
deterministic.

The fix seems wrong to me at first impression, because "LAPIC enabled"
and "irqchip not split" should imply the existence of an in-kernel
IOAPIC.  However, I cannot suggest the right course of action without
seeing a testcase.

Paolo