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? > 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
