On Thu, 25 Feb 2021 13:52:32 -0500 Steven Sistare <steven.sistare@xxxxxxxxxx> wrote: > On 2/25/2021 1:00 PM, Alex Williamson wrote: > > On Thu, 25 Feb 2021 10:25:16 -0500 > > Steven Sistare <steven.sistare@xxxxxxxxxx> wrote: > > > >> On 2/24/2021 5:55 PM, Alex Williamson wrote: > >>> On Tue, 23 Feb 2021 18:58:18 -0500 > >>> Steven Sistare <steven.sistare@xxxxxxxxxx> 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@xxxxxxxxxx> wrote: > >>>>>> > >>>>>>> On 2/23/2021 12:45 PM, Alex Williamson wrote: > >>>>>>>> On Tue, 23 Feb 2021 08:56:36 -0500 > >>>>>>>> Steven Sistare <steven.sistare@xxxxxxxxxx> wrote: > >>>>>>>> > >>>>>>>>> On 2/22/2021 6:17 PM, Alex Williamson wrote: > >>>>>>>>>> On Mon, 22 Feb 2021 15:51:45 -0700 > >>>>>>>>>> Alex Williamson <alex.williamson@xxxxxxxxxx> wrote: > >>>>>>>>>> > >>>>>>>>>>> On Mon, 22 Feb 2021 17:10:43 +0300 > >>>>>>>>>>> Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxx> > >>>>>>>>>>>> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > >>>>>>>>>>>> > >>>>>>>>>>>> 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, > >> > >> Changing size to u64 and using U64_MAX as the upper bound should do the trick: > > > > Seems reasonable. Want to slap a title, log, and sign-off on that? > > Thanks, > > See attached - Steve Thanks! Would you mind posting it inline in a new thread on the kvm and lkml lists? It makes it much easier to search for patch history later, especially since this thread isn't copying the standard vfio development lists. In light of that, the following tag would also be useful: Link: https://lore.kernel.org/linux-mm/20210222141043.GW2222@kadam/ Also, sha1 for Fixes tags are generally 12 chars. Thanks, Alex
