On Tue, Nov 01, 2022 at 01:32:23PM -0700, Nicolin Chen wrote:
> 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.
I'm surprised no compiler warned about this!
>
> > +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.
Yes, looks like
Thanks,
Jason
