Re: [RFC PATCH v2 08/11] iommufd/selftest: Add mock-domain IOVA queries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] iommufd: soft lockup in iommufd_test_md_check_mapped loop
- [Medium] iommufd: UAF race between mock page table walk and dma-buf revoke
--

commit 64ca9f35d00cc158de5f4700e220a39eb8a04615
Author: Fred Griffoul <fgriffo@amazon.co.uk>

iommufd/selftest: Add mock-domain IOVA queries

A test of memory that can be taken back or replaced does not know in
advance which frame a device reaches. IOMMU_TEST_OP_MD_CHECK_MAP only
compares against a known buffer.

Add MD_CHECK_MAPPED, which checks that a range is fully mapped or fully
unmapped, and MD_IOVA_TO_PHYS, which returns the frame behind an IOVA,
or 0 if it is unmapped. Both refuse an IOVA outside the domain's
aperture.

> diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
> index af07c642a5260..1f5cd2d00fda8 100644
> --- a/drivers/iommu/iommufd/selftest.c
> +++ b/drivers/iommu/iommufd/selftest.c
[ ... ]
> +static int iommufd_test_md_iova_to_phys(struct iommufd_ucmd *ucmd,
> +					unsigned int mockpt_id,
> +					unsigned long iova)
> +{
> +	struct iommu_test_cmd *cmd = ucmd->cmd;
> +	struct iommufd_hw_pagetable *hwpt;
> +	struct mock_iommu_domain *mock;
> +	unsigned int page_size;
> +	int rc;
> +
> +	hwpt = get_md_pagetable(ucmd, mockpt_id, &mock);
> +	if (IS_ERR(hwpt))
> +		return PTR_ERR(hwpt);
> +
> +	page_size = 1 << __ffs(mock->domain.pgsize_bitmap);
> +	if (iova % page_size || !mock_domain_covers(mock, iova, page_size)) {
> +		rc = -EINVAL;
> +		goto out_put;
> +	}
> +	down_write(mock_domain_unmap_lock(hwpt));
> +	cmd->iova_to_phys.out_phys =
> +		mock->domain.ops->iova_to_phys(&mock->domain, iova);

[Severity: Medium]
Could this page table walk race with a concurrent dma-buf revoke or memory
provider unmap?

Since those unmap operations run under their own pages mutex instead of
domains_rwsem, it seems possible for a concurrent unmap (for instance,
triggered by a fuzzer or concurrently running test) to free a page table
level while iova_to_phys() is still walking it, potentially leading to a
use-after-free.

> +	up_write(mock_domain_unmap_lock(hwpt));
> +	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));
> +out_put:
> +	iommufd_put_object(ucmd->ictx, &hwpt->obj);
> +	return rc;
> +}
> +
> +static int iommufd_test_md_check_mapped(struct iommufd_ucmd *ucmd,
> +					unsigned int mockpt_id,
> +					unsigned long iova, size_t length,
> +					bool mapped)
> +{
[ ... ]
> +	down_write(mock_domain_unmap_lock(hwpt));
> +	for (; length; length -= page_size, iova += page_size) {
> +		bool is_mapped =
> +			mock->domain.ops->iova_to_phys(&mock->domain, iova) != 0;

[Severity: Medium]
Can this loop run for an unbounded amount of time and cause a soft lockup?

Since the test ioctl interface allows very large values for length, and
page_size can be as small as 4096 bytes, this loop could iterate trillions
of times without yielding.

Because it holds domains_rwsem for writing, would this stall the CPU and
starve other users of the semaphore?

> +
> +		if (is_mapped != mapped) {
> +			rc = -ENOENT;
> +			break;
> +		}
> +	}
> +	up_write(mock_domain_unmap_lock(hwpt));
> +
> +out_put:
> +	iommufd_put_object(ucmd->ictx, &hwpt->obj);
> +	return rc;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014093733.90312-1-griffoul@gmail.com?part=8