Messages in this thread | | | Subject | Re: [PATCH] KVM/Eventfd: Avoid crash when assign and deassign same eventfd in parallel. | From | Lan Tianyu <> | Date | Tue, 19 Dec 2017 14:21:29 +0800 |
| |
Hi David: Thanks for your review.
On 2017年12月18日 16:30, David Hildenbrand wrote: > On 18.12.2017 00:40, Lan Tianyu wrote: >> Syzroot reports crash in kvm_irqfd_assign() is caused by use-after-free. >> Because kvm_irqfd_assign() and kvm_irqfd_deassign() can't run in parallel >> for same eventfd. When assign path hasn't been finished after irqfd >> has been added to kvm->irqfds.items list, another thead may deassign the >> eventfd and free struct kvm_kernel_irqfd(). This causes assign path still >> uses struct kvm_kernel_irqfd freed by deassign path. To avoid such issue, >> add "initialized" flag in the struct kvm_kernel_irqfd and check the flag before >> deactivating irqfd. If irqfd is still in initialization, deassign path >> return fault.> >> Reported-by: Dmitry Vyukov <dvyukov@google.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Radim Krčmář <rkrcmar@redhat.com> >> Cc: Dmitry Vyukov <dvyukov@google.com> >> Cc: Wanpeng Li <kernellwp@gmail.com> >> Signed-off-by: Tianyu Lan <tianyu.lan@intel.com> >> --- >> include/linux/kvm_irqfd.h | 1 + >> virt/kvm/eventfd.c | 11 +++++++++-- >> 2 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/kvm_irqfd.h b/include/linux/kvm_irqfd.h >> index 76c2fbc..be6b254 100644 >> --- a/include/linux/kvm_irqfd.h >> +++ b/include/linux/kvm_irqfd.h >> @@ -66,6 +66,7 @@ struct kvm_kernel_irqfd { >> struct work_struct shutdown; >> struct irq_bypass_consumer consumer; >> struct irq_bypass_producer *producer; >> + u8 initialized:1; >> }; >> >> #endif /* __LINUX_KVM_IRQFD_H */ >> diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c >> index a334399..80f06e6 100644 >> --- a/virt/kvm/eventfd.c >> +++ b/virt/kvm/eventfd.c >> @@ -421,6 +421,7 @@ int __attribute__((weak)) kvm_arch_update_irqfd_routing( >> } >> #endif >> >> + irqfd->initialized = 1; > > The ugly thing in kvm_irqfd_assign() is that we access irqfd without > holding a lock. I think that should rather be fixed than working around > that issue. (e.g. lock() -> lookup again -> verify still in list -> > unlock())
The new lock should be always held in assign path otherwise we need to lookup irqfds list frequently, right?
At first, I tried to use a mutex lock between assign and deassign path but assign path already involves some locks and add new lock maybe introduce dead lock. So I used flag check to replace with new lock.
> >> return 0; >> >> fail: >> @@ -525,6 +526,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm, >> { > > Which tool are you using to generate diffs? git format-patch?
Yes, I used git version 1.8.3.1 :) This also confused me. I will try newer version.
> > Mentioning, because the indicated function here .... (kvm_irqfd_deassign) > >> struct kvm_kernel_irqfd *irqfd, *tmp; >> struct eventfd_ctx *eventfd; >> + int ret = 0; >> >> eventfd = eventfd_ctx_fdget(args->fd); >> if (IS_ERR(eventfd)) >> @@ -543,7 +545,12 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm, >> write_seqcount_begin(&irqfd->irq_entry_sc); >> irqfd->irq_entry.type = 0; >> write_seqcount_end(&irqfd->irq_entry_sc); >> - irqfd_deactivate(irqfd); >> + >> + if (irqfd->initialized) >> + irqfd_deactivate(irqfd); >> + else >> + ret = -EFAULT; >> + >> } >> } >> >> @@ -557,7 +564,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm, >> */ > > and here are wrong and misleading (kvm_irqfd_deassig). (and just noticed > also in the second hunk) > >> flush_workqueue(irqfd_cleanup_wq); >> >> - return 0; >> + return ret; >> } >> >> int >> > >
-- Best regards Tianyu Lan
| |