Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] KVM: SVM: Initialize ir_list and ir_list_lock regardless
 of AVIC enablement
To:     Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org
Cc:     joro@8bytes.org
References: <20200922084446.7218-1-suravee.suthikulpanit@amd.com>
 <1b8ff096-85a4-3dda-61d3-9a44ca6bb360@amd.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <dafba8d1-8e1b-a3d3-d95f-e5581b26066d@redhat.com>
Date:   Mon, 28 Sep 2020 10:01:27 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <1b8ff096-85a4-3dda-61d3-9a44ca6bb360@amd.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 28/09/20 07:53, Suravee Suthikulpanit wrote:
> Hi,
> 
> Are there any issues or concerns about this patch?

Yes, sorry I haven't replied yet.  Looks like Linus is doing an -rc8 so
there's plenty of time to have it in 5.9.

The thing I'm wondering is, why is svm_update_pi_irte doing anything if
you don't have AVIC enabled?  In other word, this might not be the root
cause of the bug.  You always get to the "else" branch of the loop of
course, and I'm not sure how irq_set_vcpu_affinity returns something
with pi.prev_ga_tag set.

Thanks,

Paolo

> Thank you,
> Suravee
> 
> On 9/22/20 3:44 PM, Suravee Suthikulpanit wrote:
>> The struct vcpu_svm.ir_list and ir_list_lock are being accessed even when
>> AVIC is not enabled, while current code only initialize the list and
>> the lock only when AVIC is enabled. This ended up trigger NULL pointer
>> dereference bug in the function vm_ir_list_del with the following
>> call trace:
>>
>>      svm_update_pi_irte+0x3c2/0x550 [kvm_amd]
>>      ? proc_create_single_data+0x41/0x50
>>      kvm_arch_irq_bypass_add_producer+0x40/0x60 [kvm]
>>      __connect+0x5f/0xb0 [irqbypass]
>>      irq_bypass_register_producer+0xf8/0x120 [irqbypass]
>>      vfio_msi_set_vector_signal+0x1de/0x2d0 [vfio_pci]
>>      vfio_msi_set_block+0x77/0xe0 [vfio_pci]
>>      vfio_pci_set_msi_trigger+0x25c/0x2f0 [vfio_pci]
>>      vfio_pci_set_irqs_ioctl+0x88/0xb0 [vfio_pci]
>>      vfio_pci_ioctl+0x2ea/0xed0 [vfio_pci]
>>      ? alloc_file_pseudo+0xa5/0x100
>>      vfio_device_fops_unl_ioctl+0x26/0x30 [vfio]
>>      ? vfio_device_fops_unl_ioctl+0x26/0x30 [vfio]
>>      __x64_sys_ioctl+0x96/0xd0
>>      do_syscall_64+0x37/0x80
>>      entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> Therefore, move the initialziation code before checking for AVIC enabled
>> so that it is always excuted.
>>
>> Fixes: dfa20099e26e ("KVM: SVM: Refactor AVIC vcpu initialization into
>> avic_init_vcpu()")
>> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> ---
>>   arch/x86/kvm/svm/avic.c | 2 --
>>   arch/x86/kvm/svm/svm.c  | 3 +++
>>   2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index ac830cd50830..1ccf13783785 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -572,8 +572,6 @@ int avic_init_vcpu(struct vcpu_svm *svm)
>>       if (ret)
>>           return ret;
>>   -    INIT_LIST_HEAD(&svm->ir_list);
>> -    spin_lock_init(&svm->ir_list_lock);
>>       svm->dfr_reg = APIC_DFR_FLAT;
>>         return ret;
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index c44f3e9140d5..714d791fe5a5 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -1225,6 +1225,9 @@ static int svm_create_vcpu(struct kvm_vcpu *vcpu)
>>       svm_init_osvw(vcpu);
>>       vcpu->arch.microcode_version = 0x01000065;
>>   +    INIT_LIST_HEAD(&svm->ir_list);
>> +    spin_lock_init(&svm->ir_list_lock);
>> +
>>       return 0;
>>     free_page4:
>>
>