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@xxxxxxxxxx>
>> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>> Cc: Radim Krčmář <rkrcmar@xxxxxxxxxx>
>> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
>> Cc: Wanpeng Li <kernellwp@xxxxxxxxx>
>> Signed-off-by: Tianyu Lan <tianyu.lan@xxxxxxxxx>
>> ---
>> 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
