Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)'

[Steven Sistare <steven.sistare@oracle.com>]

On 2/22/2021 5:51 PM, Alex Williamson wrote:
> On Mon, 22 Feb 2021 17:10:43 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
>> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git  master
>> head:   37dfbfbdca66834bc0f64ec9b35e09ac6c8898da
>> commit: 0f53afa12baec8c00f5d1d6afb49325ada105253 [6931/12022] vfio/type1: unmap cleanup
> 
> It's always the patches that claim no functional change... ;)
> 
>> config: i386-randconfig-m021-20210222 (attached as .config)
>> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>
>> New smatch warnings:
>> drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)'
>>
>> vim +1093 drivers/vfio/vfio_iommu_type1.c
>>
>> 73fa0d10d077d9 Alex Williamson 2012-07-31  1071  static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1072  			     struct vfio_iommu_type1_dma_unmap *unmap,
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1073  			     struct vfio_bitmap *bitmap)
>> 73fa0d10d077d9 Alex Williamson 2012-07-31  1074  {
>> c086de818dd81c Kirti Wankhede  2016-11-17  1075  	struct vfio_dma *dma, *dma_last = NULL;
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1076  	size_t unmapped = 0, pgsize;
>> 0f53afa12baec8 Steve Sistare   2021-01-29  1077  	int ret = -EINVAL, retries = 0;
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1078  	unsigned long pgshift;
>> 0f53afa12baec8 Steve Sistare   2021-01-29  1079  	dma_addr_t iova = unmap->iova;
>> 0f53afa12baec8 Steve Sistare   2021-01-29  1080  	unsigned long size = unmap->size;
>>                                                         ^^^^^^^^^^^^^^^^^^
>>
>> 73fa0d10d077d9 Alex Williamson 2012-07-31  1081  
>> cade075f265b25 Kirti Wankhede  2020-05-29  1082  	mutex_lock(&iommu->lock);
>> cade075f265b25 Kirti Wankhede  2020-05-29  1083  
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1084  	pgshift = __ffs(iommu->pgsize_bitmap);
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1085  	pgsize = (size_t)1 << pgshift;
>> cade075f265b25 Kirti Wankhede  2020-05-29  1086  
>> 0f53afa12baec8 Steve Sistare   2021-01-29  1087  	if (iova & (pgsize - 1))
>> cade075f265b25 Kirti Wankhede  2020-05-29  1088  		goto unlock;
>> cade075f265b25 Kirti Wankhede  2020-05-29  1089  
>> 0f53afa12baec8 Steve Sistare   2021-01-29  1090  	if (!size || size & (pgsize - 1))
>> cade075f265b25 Kirti Wankhede  2020-05-29  1091  		goto unlock;
>> 73fa0d10d077d9 Alex Williamson 2012-07-31  1092  
>> 0f53afa12baec8 Steve Sistare   2021-01-29 @1093  	if (iova + size - 1 < iova || size > SIZE_MAX)
>>
>> size is unsigned long and SIZE_MAX is ULONG_MAX so "size > SIZE_MAX"
>> does not make sense.
> 
> I think it made sense before the above commit, where unmap->size is a
> __u64 and a user could provide a value that exceeds SIZE_MAX on ILP32.
> Seems like the fix is probably to use a size_t for the local variable
> and restore this test to compare (unmap->size > SIZE_MAX).  Steve?

That is fine, though I think using the same type as the args struct is a
more robust fix:
  u64 size;

- Steve

>> Is the " - 1" intentional on the other overflow check?  As in it's okay
>> to wrap around to zero but not further than that?  Sometimes this is
>> intentional but it requires more subsystem expertise than I possess.
> 
> Yes, since we're dealing with a start + length we need to account for
> the -1 in the end value, otherwise the user could never unmap the last
> page of the address space.  Thanks for the report!
> 
> Alex
> 
>> cade075f265b25 Kirti Wankhede  2020-05-29  1094  		goto unlock;
>> 73fa0d10d077d9 Alex Williamson 2012-07-31  1095  
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1096  	/* When dirty tracking is enabled, allow only min supported pgsize */
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1097  	if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1098  	    (!iommu->dirty_page_tracking || (bitmap->pgsize != pgsize))) {
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1099  		goto unlock;
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1100  	}
>> 73fa0d10d077d9 Alex Williamson 2012-07-31  1101  
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1102  	WARN_ON((pgsize - 1) & PAGE_MASK);
>> 331e33d2960c82 Kirti Wankhede  2020-05-29  1103  again:
>> 1ef3e2bc04223f Alex Williamson 2014-02-26  1104  	/*
>>
>> ---
>> 0-DAY CI Kernel Test Service, Intel Corporation
>> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org 
>