Re: [PATCH v5 3/3] iommufd: Add IOMMU_IOAS_RESERVE_IOVAS ioctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] iommufd: 32-bit range truncation in iommufd_ioas_reserve_iovas()
- [Low] iommufd: unnecessary iommufd_ucmd_respond() in iommufd_ioas_reserve_iovas()
--

commit c4727e768ae8fb291a064ae74e64f9f8999c92bb
Author: Guanghui Feng <guanghuifeng@linux.alibaba.com>

iommufd: Add IOMMU_IOAS_RESERVE_IOVAS ioctl

This commit adds the IOMMU_IOAS_RESERVE_IOVAS ioctl to allow userspace to
reserve IOVA ranges in an IOAS, excluding them from IOMMU_IOAS_IOVA_RANGES
and rejecting overlapping mapping operations.

> diff --git a/drivers/iommu/iommufd/ioas.c b/drivers/iommu/iommufd/ioas.c
> index 8fb5271a95060..e1ded7e8c5eed 100644
> --- a/drivers/iommu/iommufd/ioas.c
> +++ b/drivers/iommu/iommufd/ioas.c
> @@ -183,6 +183,55 @@ int iommufd_ioas_allow_iovas(struct iommufd_ucmd *ucmd)
>  	return rc;
>  }
>  
> +int iommufd_ioas_reserve_iovas(struct iommufd_ucmd *ucmd)
> +{
> +	struct iommu_ioas_reserve_iovas *cmd = ucmd->cmd;
> +	struct iommu_iova_range __user *ranges;
> +	struct iommufd_ioas *ioas;
> +	u32 i;
> +	int rc;
> +
> +	if (cmd->flags)
> +		return -EOPNOTSUPP;
> +
> +	ioas = iommufd_get_ioas(ucmd->ictx, cmd->ioas_id);
> +	if (IS_ERR(ioas))
> +		return PTR_ERR(ioas);
> +
> +	ranges = u64_to_user_ptr(cmd->ranges);
> +
> +	down_write(&ioas->iopt.iova_rwsem);
> +	for (i = 0; i < cmd->num_ranges; i++) {
> +		struct iommu_iova_range range;
> +
> +		rc = copy_from_user(&range, &ranges[i], sizeof(range));
> +		if (rc) {
> +			rc = -EFAULT;
> +			goto out_unlock;
> +		}
> +		if (range.last < range.start) {
> +			rc = -EINVAL;
> +			goto out_unlock;
> +		}
> +
> +		rc = iopt_reserve_iova(&ioas->iopt, range.start, range.last,
> +				       NULL);

[Severity: High]
On 32-bit architectures, can passing 64-bit range.start and range.last into
iopt_reserve_iova() silently truncate the range and corrupt
ioas->iopt.reserved_itree?

In struct iommu_iova_range, start and last are __aligned_u64, whereas
iopt_reserve_iova() in drivers/iommu/iommufd/io_pagetable.c and struct
interval_tree_node take unsigned long:

    int iopt_reserve_iova(struct io_pagetable *iopt, unsigned long start,
                          unsigned long last, void *owner)

Unlike iommufd_ioas_map(), iommufd_ioas_unmap(), and iommufd_ioas_copy(),
which check for ULONG_MAX overflow and return -EOVERFLOW,
iommufd_ioas_reserve_iovas() only checks range.last < range.start in 64-bit
arithmetic before passing them to iopt_reserve_iova().

On 32-bit kernels (for example, a 32-bit PAE/LPAE system where
IOMMU_GET_RECOMMENDED_RESERVATIONS reports a 64-bit PCI host bridge MMIO
window above 4 GiB), ranges with range.last > ULONG_MAX are truncated modulo
2^32, reserving the wrong low IOVA range.

Furthermore, if a range satisfies range.start <= range.last in 64-bit
arithmetic but wraps such that (unsigned long)range.last <
(unsigned long)range.start (for example, range.start = 0xfffff000ULL and
range.last = 0x100000fffULL), iopt_reserve_iova() receives start > last. The
overlap checks in iopt_area_iter_first() and iopt_allowed_iter_first() miss
existing ranges, and an inverted node with node.start > node.last is inserted
into iopt->reserved_itree, breaking interval_tree_for_each_span() in
iommufd_ioas_iova_ranges() and interval_tree_for_each_double_span() in
iopt_alloc_iova().

Should iommufd_ioas_reserve_iovas() check range.last > ULONG_MAX (or
range.start >= ULONG_MAX) and fail with -EOVERFLOW?

> +		if (rc)
> +			goto out_unlock;
> +	}
> +	up_write(&ioas->iopt.iova_rwsem);
> +
> +	rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd));

[Severity: Low]
Is the call to iommufd_ucmd_respond() needed here?

All fields of struct iommu_ioas_reserve_iovas are inputs, and
iommufd_ioas_reserve_iovas() never modifies *cmd. Calling
iommufd_ucmd_respond() performs an unnecessary copy_to_user() back to
userspace, and if the caller passes the input struct in read-only memory, the
ioctl will return -EFAULT after the reservations have already been committed
to ioas->iopt.reserved_itree.

Other input-only handlers in drivers/iommu/iommufd/ioas.c, such as
iommufd_ioas_allow_iovas() and iommufd_ioas_change_process(), return 0
directly without calling iommufd_ucmd_respond().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028071906.2325378-1-guanghuifeng@linux.alibaba.com?part=3