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). I can't say I'm really interested in adding complexity to make it work in such a case either. Maybe we can just not expose it, ex: diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index ed03f3fcb07e..6b69a74b3db0 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -1207,7 +1207,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; + size_t 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; @@ -1228,7 +1228,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu, goto unlock; } - if (iova + size - 1 < iova || size > SIZE_MAX) + if (iova + size - 1 < iova || unmap->size > SIZE_MAX) goto unlock; /* When dirty tracking is enabled, allow only min supported pgsize */ @@ -2657,9 +2657,10 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu, case VFIO_TYPE1_IOMMU: case VFIO_TYPE1v2_IOMMU: case VFIO_TYPE1_NESTING_IOMMU: - case VFIO_UNMAP_ALL: case VFIO_UPDATE_VADDR: return 1; + case VFIO_UNMAP_ALL: + return PHYS_ADDR_MAX == SIZE_MAX ? 1 : 0; case VFIO_DMA_CC_IOMMU: if (!iommu) return 0; @@ -2868,6 +2869,10 @@ static int vfio_iommu_type1_unmap_dma(struct vfio_iommu *iommu, VFIO_DMA_UNMAP_FLAG_VADDR))) return -EINVAL; + if ((PHYS_ADDR_MAX != SIZE_MAX) && + (unmap.flags & VFIO_DMA_UNMAP_FLAG_ALL)) + return -EINVAL; + if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) { unsigned long pgshift; > > 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@xxxxxxxxxxxx >
