Messages in this thread Patch in this message |  | | Subject | Re: [PATCH] KVM/Eventfd: Avoid crash when assign and deassign same eventfd in parallel. | From | David Hildenbrand <> | Date | Mon, 18 Dec 2017 09:30:23 +0100 |
| |
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())
> 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?
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 >
--
Thanks,
David / dhildenb
|  |