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

On Wed, Nov 02, 2022 at 10:17:45AM -0300, Jason Gunthorpe wrote:

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!

clang does have -Wunreachable-code-return to try and flag issues like this but it is not on by default nor included in -Wall:

https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-retu...

The fact it is included in -Wunreachable-code-aggressive makes me think that this might generate a lot of false positives around constructs such as

if (IS_ENABLED(CONFIG_...)) return ...;

return ...;

but I have not actually tested it.

...

...

+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