Re: [PATCH] KVM: PPC: Book3S: Fix race and leak in kvm_vm_ioctl_create_spapr_tce()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4151 bytes --]

On Thu, Aug 24, 2017 at 01:40:08PM +1000, Paul Mackerras wrote:
> Nixiaoming pointed out that there is a memory leak in
> kvm_vm_ioctl_create_spapr_tce() if the call to anon_inode_getfd()
> fails; the memory allocated for the kvmppc_spapr_tce_table struct
> is not freed, and nor are the pages allocated for the iommu
> tables.  In addition, we have already incremented the process's
> count of locked memory pages, and this doesn't get restored on
> error.
> 
> David Hildenbrand pointed out that there is a race in that the
> function checks early on that there is not already an entry in the
> stt->iommu_tables list with the same LIOBN, but an entry with the
> same LIOBN could get added between then and when the new entry is
> added to the list.
> 
> This fixes all three problems.  To simplify things, we now call
> anon_inode_getfd() before placing the new entry in the list.  The
> check for an existing entry is done while holding the kvm->lock
> mutex, immediately before adding the new entry to the list.
> Finally, on failure we now call kvmppc_account_memlimit to
> decrement the process's count of locked memory pages.
> 
> Reported-by: Nixiaoming <nixiaoming@huawei.com>
> Reported-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  arch/powerpc/kvm/book3s_64_vio.c | 55 ++++++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> index a160c14304eb..d463c1cd0d8d 100644
> --- a/arch/powerpc/kvm/book3s_64_vio.c
> +++ b/arch/powerpc/kvm/book3s_64_vio.c
> @@ -297,29 +297,22 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
>  	unsigned long npages, size;
>  	int ret = -ENOMEM;
>  	int i;
> +	int fd = -1;
>  
>  	if (!args->size)
>  		return -EINVAL;
>  
> -	/* Check this LIOBN hasn't been previously allocated */
> -	list_for_each_entry(stt, &kvm->arch.spapr_tce_tables, list) {
> -		if (stt->liobn == args->liobn)
> -			return -EBUSY;
> -	}
> -
>  	size = _ALIGN_UP(args->size, PAGE_SIZE >> 3);
>  	npages = kvmppc_tce_pages(size);
>  	ret = kvmppc_account_memlimit(kvmppc_stt_pages(npages), true);
> -	if (ret) {
> -		stt = NULL;
> -		goto fail;
> -	}
> +	if (ret)
> +		return ret;
>  
>  	ret = -ENOMEM;
>  	stt = kzalloc(sizeof(*stt) + npages * sizeof(struct page *),
>  		      GFP_KERNEL);
>  	if (!stt)
> -		goto fail;
> +		goto fail_acct;
>  
>  	stt->liobn = args->liobn;
>  	stt->page_shift = args->page_shift;
> @@ -334,24 +327,42 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,
>  			goto fail;
>  	}
>  
> -	kvm_get_kvm(kvm);
> +	ret = fd = anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops,
> +				    stt, O_RDWR | O_CLOEXEC);
> +	if (ret < 0)
> +		goto fail;
>  
>  	mutex_lock(&kvm->lock);
> -	list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);
> +
> +	/* Check this LIOBN hasn't been previously allocated */
> +	ret = 0;
> +	list_for_each_entry(stt, &kvm->arch.spapr_tce_tables, list) {
> +		if (stt->liobn == args->liobn) {
> +			ret = -EBUSY;
> +			break;
> +		}
> +	}
> +
> +	if (!ret) {
> +		list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);
> +		kvm_get_kvm(kvm);
> +	}
>  
>  	mutex_unlock(&kvm->lock);
>  
> -	return anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops,
> -				stt, O_RDWR | O_CLOEXEC);
> +	if (!ret)
> +		return fd;
>  
> -fail:
> -	if (stt) {
> -		for (i = 0; i < npages; i++)
> -			if (stt->pages[i])
> -				__free_page(stt->pages[i]);
> +	put_unused_fd(fd);
>  
> -		kfree(stt);
> -	}
> + fail:
> +	for (i = 0; i < npages; i++)
> +		if (stt->pages[i])
> +			__free_page(stt->pages[i]);
> +
> +	kfree(stt);
> + fail_acct:
> +	kvmppc_account_memlimit(kvmppc_stt_pages(npages), false);
>  	return ret;
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]