Re: [PATCH] KVM/Eventfd: Avoid crash when assign and deassign same eventfd in parallel.

[Lan Tianyu]

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