Re: [PATCH v3 15/15] iommufd: Add a selftest

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 25, 2022 at 03:12:24PM -0300, Jason Gunthorpe wrote:
 
> diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c

> +static inline struct iommufd_hw_pagetable *
> +get_md_pagetable(struct iommufd_ucmd *ucmd, u32 mockpt_id,
> +		 struct mock_iommu_domain **mock)
> +{
> +	struct iommufd_hw_pagetable *hwpt;
> +	struct iommufd_object *obj;
> +
> +	obj = iommufd_get_object(ucmd->ictx, mockpt_id,
> +				 IOMMUFD_OBJ_HW_PAGETABLE);
> +	if (IS_ERR(obj))
> +		return ERR_CAST(obj);
> +	hwpt = container_of(obj, struct iommufd_hw_pagetable, obj);
> +	if (hwpt->domain->ops != mock_ops.default_domain_ops) {
> +		return ERR_PTR(-EINVAL);
> +		iommufd_put_object(&hwpt->obj);

Coverity reports that return is placed before iommufd_put_object.

> +static int iommufd_test_access_pages(struct iommufd_ucmd *ucmd,
> +				     unsigned int access_id, unsigned long iova,
> +				     size_t length, void __user *uptr,
> +				     u32 flags)
> +{
> +	struct iommu_test_cmd *cmd = ucmd->cmd;
> +	struct selftest_access_item *item;
> +	struct selftest_access *staccess;
> +	struct page **pages;
> +	size_t npages;
> +	int rc;
> +
> +	if (flags & ~MOCK_FLAGS_ACCESS_WRITE)
> +		return -EOPNOTSUPP;
> +
> +	staccess = iommufd_access_get(access_id);
> +	if (IS_ERR(staccess))
> +		return PTR_ERR(staccess);
> +
> +	npages = (ALIGN(iova + length, PAGE_SIZE) -
> +		  ALIGN_DOWN(iova, PAGE_SIZE)) /
> +		 PAGE_SIZE;
> +	pages = kvcalloc(npages, sizeof(*pages), GFP_KERNEL_ACCOUNT);
> +	if (!pages) {
> +		rc = -ENOMEM;
> +		goto out_put;
> +	}
> +
> +	rc = iommufd_access_pin_pages(staccess->access, iova, length, pages,
> +				      flags & MOCK_FLAGS_ACCESS_WRITE);
> +	if (rc)
> +		goto out_free_pages;
> +
> +	rc = iommufd_test_check_pages(
> +		uptr - (iova - ALIGN_DOWN(iova, PAGE_SIZE)), pages, npages);
> +	if (rc)
> +		goto out_unaccess;
> +
> +	item = kzalloc(sizeof(*item), GFP_KERNEL_ACCOUNT);
> +	if (!item) {
> +		rc = -ENOMEM;
> +		goto out_unaccess;
> +	}
> +
> +	item->iova = iova;
> +	item->length = length;
> +	spin_lock(&staccess->lock);
> +	item->id = staccess->next_id++;
> +	list_add_tail(&item->items_elm, &staccess->items);
> +	spin_unlock(&staccess->lock);
> +
> +	cmd->access_pages.out_access_item_id = item->id;
> +	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> +	if (rc)
> +		goto out_free_item;
> +	goto out_free_pages;
> +
> +out_free_item:
> +	spin_lock(&staccess->lock);
> +	list_del(&item->items_elm);
> +	spin_unlock(&staccess->lock);
> +	kfree(item);
> +out_unaccess:
> +	iommufd_access_unpin_pages(staccess->access, iova, length);
> +out_free_pages:
> +	kvfree(pages);

Coverity reports a double free here, call trace:

[jumped from] rc = iommufd_access_pin_pages(..., pages, ...);
	[in which] iopt_pages_add_access(..., out_pages, ...);
		[then] iopt_pages_fill_xarray(..., out_pages);
			[then] iopt_pages_fill_from_mm(..., out_pages);
				[then] user->upages = out_pages + ...;
				       pfn_reader_user_pin(user, ...);
					[then] kfree(user->upages);
					       return -EFAULT;

Should be the same potential issue in the other email.



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux