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: diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 6cf1dad..b1be0a6 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -181,7 +181,7 @@ static struct vfio_dma *vfio_find_dma(struct vfio_iommu *iommu, } static struct rb_node *vfio_find_dma_first_node(struct vfio_iommu *iommu, - dma_addr_t start, size_t size) + dma_addr_t start, u64 size) { struct rb_node *res = NULL; struct rb_node *node = iommu->dma_list.rb_node; @@ -1184,7 +1184,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, int ret = -EINVAL, retries = 0; unsigned long pgshift; dma_addr_t iova = unmap->iova; - unsigned long size = unmap->size; + u64 size = unmap->size; bool unmap_all = unmap->flags & VFIO_DMA_UNMAP_FLAG_ALL; bool invalidate_vaddr = unmap->flags & VFIO_DMA_UNMAP_FLAG_VADDR; struct rb_node *n, *first_n; @@ -1200,14 +1200,12 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, if (unmap_all) { if (iova || size) goto unlock; - size = SIZE_MAX; - } else if (!size || size & (pgsize - 1)) { + size = U64_MAX; + } else if (!size || size & (pgsize - 1) || + iova + size - 1 < iova || size > SIZE_MAX) { goto unlock; } - if (iova + size - 1 < iova || size > SIZE_MAX) - goto unlock; - /* When dirty tracking is enabled, allow only min supported pgsize */ if ((unmap->flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) && (!iommu->dirty_page_tracking || (bitmap->pgsize != pgsize))) {
