Re: [PATCH 2/2] iommu/dma: Handle potential overflow in iommu_dma_init_domain

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

On 18/12/2018 18:48, Andrew Jones wrote:
> The sum of base and size may overflow, particularly considering there
> are cases where size will be U64_MAX. Also, end_pfn is unused, so we
> remove it. Finally, as size doesn't actually need to be IOMMU page
> aligned we remove it from the comment stating both it and base should
> be. I wonder if we shouldn't at least warn when base is not aligned?

TBH if we're going to do anything here we may as well just get rid of 
size altogether. It's pretty unrealistic that the check it's used in 
would ever actually fail, and even if a sufficiently weird system did 
exist for that to happen, I don't think it would make much practical 
difference to just carry on at this point and let DMA mapping calls fail 
later.

Robin.

> 
> Signed-off-by: Andrew Jones <drjones-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>   drivers/iommu/dma-iommu.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index d1b04753b204..a0b01398b15c 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -281,9 +281,9 @@ static void iommu_dma_flush_iotlb_all(struct iova_domain *iovad)
>    * @size: Size of IOVA space
>    * @dev: Device the domain is being initialised for
>    *
> - * @base and @size should be exact multiples of IOMMU page granularity to
> - * avoid rounding surprises. If necessary, we reserve the page at address 0
> - * to ensure it is an invalid IOVA. It is safe to reinitialise a domain, but
> + * @base should be an exact multiple of IOMMU page granularity to avoid
> + * rounding surprises. If necessary, we reserve the page at address 0 to
> + * ensure it is an invalid IOVA. It is safe to reinitialise a domain, but
>    * any change which could make prior IOVAs invalid will fail.
>    */
>   int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
> @@ -291,21 +291,24 @@ int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
>   {
>   	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>   	struct iova_domain *iovad = &cookie->iovad;
> -	unsigned long order, base_pfn, end_pfn;
> +	dma_addr_t max_addr = base + size - 1;
> +	unsigned long order, base_pfn;
>   	int attr;
>   
>   	if (!cookie || cookie->type != IOMMU_DMA_IOVA_COOKIE)
>   		return -EINVAL;
>   
> +	if (max_addr < base)
> +		max_addr = U64_MAX;
> +
>   	/* Use the smallest supported page size for IOVA granularity */
>   	order = __ffs(domain->pgsize_bitmap);
>   	base_pfn = max_t(unsigned long, 1, base >> order);
> -	end_pfn = (base + size - 1) >> order;
>   
>   	/* Check the domain allows at least some access to the device... */
>   	if (domain->geometry.force_aperture) {
>   		if (base > domain->geometry.aperture_end ||
> -		    base + size <= domain->geometry.aperture_start) {
> +		    max_addr < domain->geometry.aperture_start) {
>   			pr_warn("specified DMA range outside IOMMU capability\n");
>   			return -EFAULT;
>   		}
>