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)'

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 23 Feb 2021 18:58:18 -0500
Steven Sistare <steven.sistare@oracle.com> wrote:

> On 2/23/2021 4:52 PM, Steven Sistare wrote:
> > On 2/23/2021 4:10 PM, Alex Williamson wrote:  
> >> On Tue, 23 Feb 2021 15:37:31 -0500
> >> Steven Sistare <steven.sistare@oracle.com> wrote:
> >>  
> >>> On 2/23/2021 12:45 PM, Alex Williamson wrote:  
> >>>> On Tue, 23 Feb 2021 08:56:36 -0500
> >>>> Steven Sistare <steven.sistare@oracle.com> wrote:
> >>>>     
> >>>>> On 2/22/2021 6:17 PM, Alex Williamson wrote:    
> >>>>>> On Mon, 22 Feb 2021 15:51:45 -0700
> >>>>>> Alex Williamson <alex.williamson@redhat.com> 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?      
> >>>>>>
> >>>>>> Actually it seems like VFIO_DMA_UNMAP_FLAG_ALL doesn't work when
> >>>>>> PHYS_ADDR_MAX != SIZE_MAX (ex. x86 PAE - I think).        
> >>>>>
> >>>>> It seems like PAE causes problems even before VFIO_DMA_UNMAP_FLAG_ALL.    
> >>>>
> >>>> This wouldn't surprise me, I don't know of any actual non-64bit users
> >>>> and pure 32bit support was only lightly validated ages ago.
> >>>>     
> >>>>> In the previous vfio_dma_do_unmap code, the u64 unmap->size would be
> >>>>> truncated when passed to vfio_find_dma.    
> >>>>
> >>>> We would have failed with -EINVAL before we get there due to this
> >>>> SIZE_MAX test.  I think the existing (previous) PAE interface is at
> >>>> least self consistent; I see the mapping path also attempts to check
> >>>> that casting map->size as size_t still matches the original value.    
> >>>
> >>> Good point, and it also checks for vaddr and iova overflow and wrap:
> >>>
> >>> vfio_dma_do_map()
> >>>         if (map->size != size || map->vaddr != vaddr || map->iova != iova)
> >>>                 return -EINVAL;
> >>>         if (iova + size - 1 < iova || vaddr + size - 1 < vaddr) {
> >>>                 ret = -EINVAL;
> >>>
> >>> With that, I don't see a problem with PAE, for unmap-all or otherwise.
> >>> We just need "u64 size" in vfio_dma_do_unmap to avoid the smatch warning.  
> >>
> >> I'm not convinced.  My understanding is that on PAE phys_addr_t is
> >> 64-bit while size_t is 32-bit.  dma_addr_t (iova above) seems to follow
> >> phys_addr_t.  That suggests to me that our {un}map.iova lives in a
> >> 64-bit address space, but each mapping is limited to 32-bits.  The  
> > 
> > OK, the "map->iova != iova" test does not help because dma_addr_t is 64-bit. My bad.
> > So, I re-propose my fix for unmap-all from previous email.
> > 
> > I am not keen on proposing a fix for the potential legacy bugs, vfio_find_dma() and
> > its callers, if no one is reporting bugs and no one uses it with vfio.  It has the 
> > potential for regression with no upside.  
> 
> ... but there are no legacy bugs because size is constrained to 32-bits in do_map as
> you pointed out, so all calls to vfio_find_dma are safe.

Right, all legacy call paths are ok afaict, but the unmap-all flag
can't reach any mappings if there are none below an iova of SIZE_MAX.
We should either fix vfio_find_first_dma_node() for this scenario or
disable unmap-all where this is a possibility.  Thanks,

Alex