* [PATCH] KVM: SVM: Initialize ir_list and ir_list_lock regardless of AVIC enablement
@ 2020-09-22 8:44 Suravee Suthikulpanit
2020-09-28 5:53 ` Suravee Suthikulpanit
0 siblings, 1 reply; 4+ messages in thread
From: Suravee Suthikulpanit @ 2020-09-22 8:44 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: pbonzini, joro, Suravee Suthikulpanit
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:
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] KVM: SVM: Initialize ir_list and ir_list_lock regardless of AVIC enablement
2020-09-22 8:44 [PATCH] KVM: SVM: Initialize ir_list and ir_list_lock regardless of AVIC enablement Suravee Suthikulpanit
@ 2020-09-28 5:53 ` Suravee Suthikulpanit
2020-09-28 8:01 ` Paolo Bonzini
0 siblings, 1 reply; 4+ messages in thread
From: Suravee Suthikulpanit @ 2020-09-28 5:53 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: pbonzini, joro
Hi,
Are there any issues or concerns about this patch?
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:
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] KVM: SVM: Initialize ir_list and ir_list_lock regardless of AVIC enablement
2020-09-28 5:53 ` Suravee Suthikulpanit
@ 2020-09-28 8:01 ` Paolo Bonzini
2020-10-03 23:27 ` Suravee Suthikulpanit
0 siblings, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2020-09-28 8:01 UTC (permalink / raw)
To: Suravee Suthikulpanit, linux-kernel, kvm; +Cc: joro
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:
>>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] KVM: SVM: Initialize ir_list and ir_list_lock regardless of AVIC enablement
2020-09-28 8:01 ` Paolo Bonzini
@ 2020-10-03 23:27 ` Suravee Suthikulpanit
0 siblings, 0 replies; 4+ messages in thread
From: Suravee Suthikulpanit @ 2020-10-03 23:27 UTC (permalink / raw)
To: Paolo Bonzini, linux-kernel, kvm; +Cc: joro
Paolo,
On 9/28/20 3:01 PM, Paolo Bonzini wrote:
> 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.
You are right. pi_prev_ga_tag needs to be initialized before used
(in case AVIC is not enabled). I have already sent out another patch
to properly fix the issue instead with subject
(KVM: SVM: Initialize prev_ga_tag before use).
Thanks,
Suravee
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-10-03 23:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-22 8:44 [PATCH] KVM: SVM: Initialize ir_list and ir_list_lock regardless of AVIC enablement Suravee Suthikulpanit
2020-09-28 5:53 ` Suravee Suthikulpanit
2020-09-28 8:01 ` Paolo Bonzini
2020-10-03 23:27 ` Suravee Suthikulpanit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.