| Message ID | 1513554007-12302-1-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >> > >
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; return 0; fail: @@ -525,6 +526,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm, { 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, */ flush_workqueue(irqfd_cleanup_wq); - return 0; + return ret; } int
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(-)