On 2/22/2021 5:51 PM, Alex Williamson 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? That is fine, though I think using the same type as the args struct is a more robust fix: u64 size; - 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 >
