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

From: Nathan Chancellor <nathan@kernel.org>
To: Jason Gunthorpe <jgg@nvidia.com>
Cc: Nicolin Chen <nicolinc@nvidia.com>,
	Lu Baolu <baolu.lu@linux.intel.com>,
	bpf@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
	David Woodhouse <dwmw2@infradead.org>,
	iommu@lists.linux.dev, Joerg Roedel <joro@8bytes.org>,
	Kevin Tian <kevin.tian@intel.com>,
	linux-doc@vger.kernel.org, linux-kselftest@vger.kernel.org,
	llvm@lists.linux.dev, Nick Desaulniers <ndesaulniers@google.com>,
	Miguel Ojeda <ojeda@kernel.org>,
	Robin Murphy <robin.murphy@arm.com>,
	Shuah Khan <shuah@kernel.org>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	Tom Rix <trix@redhat.com>, Will Deacon <will@kernel.org>,
	Alex Williamson <alex.williamson@redhat.com>,
	Chaitanya Kulkarni <chaitanyak@nvidia.com>,
	Cornelia Huck <cohuck@redhat.com>,
	Daniel Jordan <daniel.m.jordan@oracle.com>,
	David Gibson <david@gibson.dropbear.id.au>,
	Eric Auger <eric.auger@redhat.com>,
	Eric Farman <farman@linux.ibm.com>,
	Jason Wang <jasowang@redhat.com>,
	Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Joao Martins <joao.m.martins@oracle.com>,
	kvm@vger.kernel.org, Matthew Rosato <mjrosato@linux.ibm.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Niklas Schnelle <schnelle@linux.ibm.com>,
	Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
	Yi Liu <yi.l.liu@intel.com>, Keqian Zhu <zhukeqian1@huawei.com>
Subject: Re: [PATCH v3 15/15] iommufd: Add a selftest
Date: Wed, 2 Nov 2022 11:49:48 -0700	[thread overview]
Message-ID: <Y2K7zMvzrHmQJ1hk@dev-arch.thelio-3990X> (raw)
In-Reply-To: <Y2Jt+WxNUwROJ8fN@nvidia.com>

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-return

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