Re: [RFC PATCH 09/14] iommufd: Map guest_memfd files

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] iommufd: missing iova page-alignment check for gmem in iopt_insert_area()
--

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

iommufd: Map guest_memfd files

This commit allows IOMMU_IOAS_MAP_FILE to map guest_memfd files using
guest_memfd's export interface without pinning pages. It maps PAGE_SIZE
entries and updates IOMMU mappings upon guest_memfd update notifications.

> diff --git a/drivers/iommu/iommufd/io_pagetable.c b/drivers/iommu/iommufd/io_pagetable.c
> index 43d518e4ce585..62fa553cdffc0 100644
> --- a/drivers/iommu/iommufd/io_pagetable.c
> +++ b/drivers/iommu/iommufd/io_pagetable.c
> @@ -217,9 +217,13 @@ static int iopt_insert_area(struct io_pagetable *iopt, struct iopt_area *area,
>  		return -EPERM;
>  
>  	area->iommu_prot = iommu_prot;
> +	area->gmem = iopt_is_gmem(pages);
>  	area->page_offset = start_byte % PAGE_SIZE;
>  	if (area->page_offset & (iopt->iova_alignment - 1))
>  		return -EINVAL;
> +	/* guest_memfd frames are mapped whole, see iopt_gmem_map(). */
> +	if (area->gmem && (area->page_offset || !PAGE_ALIGNED(length)))
> +		return -EINVAL;

[Severity: Medium]
Should iopt_insert_area() also check !PAGE_ALIGNED(iova) when area->gmem
is set?

When userspace calls IOMMU_IOAS_MAP_FILE (or IOMMU_IOAS_COPY) with
IOMMU_IOAS_MAP_FIXED_IOVA, iopt_alloc_area_pages() validates the IOVA
through iopt_check_iova(), which only checks iova & (iopt->iova_alignment - 1).
Unlike iopt->disable_large_pages in iopt_calculate_iova_alignment(), a
guest_memfd area does not force iopt->iova_alignment to PAGE_SIZE, so
iopt->iova_alignment is 1 when no domain is attached yet, or smaller than
PAGE_SIZE when attached to a domain supporting sub-PAGE_SIZE granules:

drivers/iommu/iommufd/io_pagetable.c:iopt_calculate_iova_alignment() {
	/* See batch_iommu_map_small() */
	if (iopt->disable_large_pages)
		new_iova_alignment = PAGE_SIZE;
	else
		new_iova_alignment = 1;
}

Later, iopt_gmem_map() derives iova from iopt_area_index_to_iova(area, index)
and passes it to batch_iommu_map_small(), which expects PAGE_SIZE alignment:

drivers/iommu/iommufd/pages.c:batch_iommu_map_small() {
	if (IS_ENABLED(CONFIG_IOMMUFD_TEST))
		WARN_ON(paddr % PAGE_SIZE || iova % PAGE_SIZE ||
			size % PAGE_SIZE);

	while (size) {
		rc = iommu_map(domain, iova, paddr, PAGE_SIZE, prot,
			       GFP_KERNEL_ACCOUNT);
}

This allows userspace to trigger the WARN_ON() in batch_iommu_map_small()
when CONFIG_IOMMUFD_TEST is enabled, or pass an unaligned iova with
PAGE_SIZE to iommu_map() during mapping or asynchronously in
iopt_gmem_update().

Also, in iopt_alloc_area_pages():

> @@ -289,6 +293,9 @@ static int iopt_alloc_area_pages(struct io_pagetable *iopt,
>  		case IOPT_ADDRESS_DMABUF:
>  			start = elm->start_byte + elm->pages->dmabuf.start;
>  			break;
> +		case IOPT_ADDRESS_GMEM:
> +			start = elm->start_byte + elm->pages->gmem.start;
> +			break;
>  		}
>  		rc = iopt_alloc_iova(iopt, dst_iova, start, length);

Because iopt_alloc_gmem_pages() sets pages->gmem.start = start without
subtracting start_byte (unlike iopt_alloc_file_pages() and
iopt_alloc_dmabuf_pages()), if an unaligned start is passed without
IOMMU_IOAS_MAP_FIXED_IOVA, start_byte is added twice here before
iopt_insert_area() rejects area->page_offset. Should iopt_alloc_gmem_pages()
reject unaligned start or strip start_byte up front?

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