* [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)'
@ 2021-02-22 14:10 Dan Carpenter
2021-02-22 22:51 ` Alex Williamson
0 siblings, 1 reply; 15+ messages in thread
From: Dan Carpenter @ 2021-02-22 14:10 UTC (permalink / raw)
To: kbuild, Steve Sistare
Cc: lkp, kbuild-all, Linux Memory Management List, Alex Williamson,
Cornelia Huck
[-- Attachment #1: Type: text/plain, Size: 3698 bytes --]
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
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@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
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.
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.
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@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 41675 bytes --]
[-- Attachment #3: Type: text/plain, Size: 149 bytes --]
_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-22 14:10 [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' Dan Carpenter @ 2021-02-22 22:51 ` Alex Williamson 2021-02-22 23:17 ` Alex Williamson 2021-02-23 13:20 ` Steven Sistare 0 siblings, 2 replies; 15+ messages in thread From: Alex Williamson @ 2021-02-22 22:51 UTC (permalink / raw) To: Dan Carpenter Cc: kbuild, Steve Sistare, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On Mon, 22 Feb 2021 17:10:43 +0300 Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > 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@lists.01.org ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-22 22:51 ` Alex Williamson @ 2021-02-22 23:17 ` Alex Williamson 2021-02-23 13:56 ` Steven Sistare 2021-02-23 13:20 ` Steven Sistare 1 sibling, 1 reply; 15+ messages in thread From: Alex Williamson @ 2021-02-22 23:17 UTC (permalink / raw) To: Dan Carpenter Cc: kbuild, Steve Sistare, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On Mon, 22 Feb 2021 15:51:45 -0700 Alex Williamson <alex.williamson@redhat.com> wrote: > On Mon, 22 Feb 2021 17:10:43 +0300 > Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > 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@lists.01.org > ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-22 23:17 ` Alex Williamson @ 2021-02-23 13:56 ` Steven Sistare 2021-02-23 17:45 ` Alex Williamson 0 siblings, 1 reply; 15+ messages in thread From: Steven Sistare @ 2021-02-23 13:56 UTC (permalink / raw) To: Alex Williamson, Dan Carpenter Cc: kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On 2/22/2021 6:17 PM, Alex Williamson wrote: > On Mon, 22 Feb 2021 15:51:45 -0700 > Alex Williamson <alex.williamson@redhat.com> wrote: > >> On Mon, 22 Feb 2021 17:10:43 +0300 >> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> >>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>> >>> 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. In the previous vfio_dma_do_unmap code, the u64 unmap->size would be truncated when passed to vfio_find_dma. For unmap, these fixes should suffice, and I would rather do this than disable the unmap-all flag for a corner case: vfio_dma_do_unmap() size_t unmapped = 0; unsigned long size = unmap->size; ==> u64 unmapped = 0; u64 size = unmap->size; static struct rb_node *vfio_find_dma_first_node( struct vfio_iommu *iommu, dma_addr_t start, size_t size) ==> static struct rb_node *vfio_find_dma_first_node( struct vfio_iommu *iommu, dma_addr_t start, u64 size) And maybe use dma_addr_t instead of u64 in the above (which is 64 bits for CONFIG_X86_PAE). However, there are other places in the existing code that need tweaking to be safe for PAE, the vfio_find_dma() size arg for one. - Steve > 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@lists.01.org >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-23 13:56 ` Steven Sistare @ 2021-02-23 17:45 ` Alex Williamson 2021-02-23 20:37 ` Steven Sistare 0 siblings, 1 reply; 15+ messages in thread From: Alex Williamson @ 2021-02-23 17:45 UTC (permalink / raw) To: Steven Sistare Cc: Dan Carpenter, kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On Tue, 23 Feb 2021 08:56:36 -0500 Steven Sistare <steven.sistare@oracle.com> wrote: > On 2/22/2021 6:17 PM, Alex Williamson wrote: > > On Mon, 22 Feb 2021 15:51:45 -0700 > > Alex Williamson <alex.williamson@redhat.com> wrote: > > > >> On Mon, 22 Feb 2021 17:10:43 +0300 > >> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> > >>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >>> > >>> 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. > For unmap, these fixes should suffice, and I would rather do this than > disable the unmap-all flag for a corner case: > > vfio_dma_do_unmap() > size_t unmapped = 0; > unsigned long size = unmap->size; > ==> > u64 unmapped = 0; > u64 size = unmap->size; > > static struct rb_node *vfio_find_dma_first_node( > struct vfio_iommu *iommu, dma_addr_t start, size_t size) > ==> > static struct rb_node *vfio_find_dma_first_node( > struct vfio_iommu *iommu, dma_addr_t start, u64 size) > > And maybe use dma_addr_t instead of u64 in the above (which is 64 bits for > CONFIG_X86_PAE). > > However, there are other places in the existing code that need tweaking > to be safe for PAE, the vfio_find_dma() size arg for one. Yes, it looks like the IOMMU aperture checking using vfio_find_dma() could have issues where dma_addr_t > size_t. Do you want to propose a patch? Thanks, Alex > > 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@lists.01.org > >> > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-23 17:45 ` Alex Williamson @ 2021-02-23 20:37 ` Steven Sistare 2021-02-23 21:10 ` Alex Williamson 0 siblings, 1 reply; 15+ messages in thread From: Steven Sistare @ 2021-02-23 20:37 UTC (permalink / raw) To: Alex Williamson Cc: Dan Carpenter, kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On 2/23/2021 12:45 PM, Alex Williamson wrote: > On Tue, 23 Feb 2021 08:56:36 -0500 > Steven Sistare <steven.sistare@oracle.com> wrote: > >> On 2/22/2021 6:17 PM, Alex Williamson wrote: >>> On Mon, 22 Feb 2021 15:51:45 -0700 >>> Alex Williamson <alex.williamson@redhat.com> wrote: >>> >>>> On Mon, 22 Feb 2021 17:10:43 +0300 >>>> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> >>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>>>> >>>>> 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. - Steve >> For unmap, these fixes should suffice, and I would rather do this than >> disable the unmap-all flag for a corner case: >> >> vfio_dma_do_unmap() >> size_t unmapped = 0; >> unsigned long size = unmap->size; >> ==> >> u64 unmapped = 0; >> u64 size = unmap->size; >> >> static struct rb_node *vfio_find_dma_first_node( >> struct vfio_iommu *iommu, dma_addr_t start, size_t size) >> ==> >> static struct rb_node *vfio_find_dma_first_node( >> struct vfio_iommu *iommu, dma_addr_t start, u64 size) >> >> And maybe use dma_addr_t instead of u64 in the above (which is 64 bits for >> CONFIG_X86_PAE). >> >> However, there are other places in the existing code that need tweaking >> to be safe for PAE, the vfio_find_dma() size arg for one. > > Yes, it looks like the IOMMU aperture checking using vfio_find_dma() > could have issues where dma_addr_t > size_t. Do you want to propose a > patch? Thanks, > > Alex > >>> 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@lists.01.org >>>> >>> >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-23 20:37 ` Steven Sistare @ 2021-02-23 21:10 ` Alex Williamson 2021-02-23 21:52 ` Steven Sistare 0 siblings, 1 reply; 15+ messages in thread From: Alex Williamson @ 2021-02-23 21:10 UTC (permalink / raw) To: Steven Sistare Cc: Dan Carpenter, kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On Tue, 23 Feb 2021 15:37:31 -0500 Steven Sistare <steven.sistare@oracle.com> wrote: > On 2/23/2021 12:45 PM, Alex Williamson wrote: > > On Tue, 23 Feb 2021 08:56:36 -0500 > > Steven Sistare <steven.sistare@oracle.com> wrote: > > > >> On 2/22/2021 6:17 PM, Alex Williamson wrote: > >>> On Mon, 22 Feb 2021 15:51:45 -0700 > >>> Alex Williamson <alex.williamson@redhat.com> wrote: > >>> > >>>> On Mon, 22 Feb 2021 17:10:43 +0300 > >>>> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> > >>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >>>>> > >>>>> 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 unmap-all logic only looks for a first entry to unmap in the [0..SIZE_MAX] range. If an entry happens to exist there, we'll unmap everything, but the user would have no requirement to have a mapping within that range, their first mapping could exist at iova = (SIZE_MAX + 1). So unmap-all would effectively need a special case to use rb_first(), which mostly negates the reason we added vfio_find_dma_first_node(). Right? Thanks, Alex > >> For unmap, these fixes should suffice, and I would rather do this than > >> disable the unmap-all flag for a corner case: > >> > >> vfio_dma_do_unmap() > >> size_t unmapped = 0; > >> unsigned long size = unmap->size; > >> ==> > >> u64 unmapped = 0; > >> u64 size = unmap->size; > >> > >> static struct rb_node *vfio_find_dma_first_node( > >> struct vfio_iommu *iommu, dma_addr_t start, size_t size) > >> ==> > >> static struct rb_node *vfio_find_dma_first_node( > >> struct vfio_iommu *iommu, dma_addr_t start, u64 size) > >> > >> And maybe use dma_addr_t instead of u64 in the above (which is 64 bits for > >> CONFIG_X86_PAE). > >> > >> However, there are other places in the existing code that need tweaking > >> to be safe for PAE, the vfio_find_dma() size arg for one. > > > > Yes, it looks like the IOMMU aperture checking using vfio_find_dma() > > could have issues where dma_addr_t > size_t. Do you want to propose a > > patch? Thanks, > > > > Alex > > > >>> 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@lists.01.org > >>>> > >>> > >> > > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-23 21:10 ` Alex Williamson @ 2021-02-23 21:52 ` Steven Sistare 2021-02-23 23:58 ` Steven Sistare 0 siblings, 1 reply; 15+ messages in thread From: Steven Sistare @ 2021-02-23 21:52 UTC (permalink / raw) To: Alex Williamson Cc: Dan Carpenter, kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On 2/23/2021 4:10 PM, Alex Williamson wrote: > On Tue, 23 Feb 2021 15:37:31 -0500 > Steven Sistare <steven.sistare@oracle.com> wrote: > >> On 2/23/2021 12:45 PM, Alex Williamson wrote: >>> On Tue, 23 Feb 2021 08:56:36 -0500 >>> Steven Sistare <steven.sistare@oracle.com> wrote: >>> >>>> On 2/22/2021 6:17 PM, Alex Williamson wrote: >>>>> On Mon, 22 Feb 2021 15:51:45 -0700 >>>>> Alex Williamson <alex.williamson@redhat.com> wrote: >>>>> >>>>>> On Mon, 22 Feb 2021 17:10:43 +0300 >>>>>> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> >>>>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>>>>>> >>>>>>> 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. - Steve > unmap-all logic only looks for a first entry to unmap in the > [0..SIZE_MAX] range. If an entry happens to exist there, we'll unmap > everything, but the user would have no requirement to have a mapping > within that range, their first mapping could exist at iova = (SIZE_MAX > + 1). So unmap-all would effectively need a special case to use > rb_first(), which mostly negates the reason we added > vfio_find_dma_first_node(). Right? Thanks, > > Alex > > >>>> For unmap, these fixes should suffice, and I would rather do this than >>>> disable the unmap-all flag for a corner case: >>>> >>>> vfio_dma_do_unmap() >>>> size_t unmapped = 0; >>>> unsigned long size = unmap->size; >>>> ==> >>>> u64 unmapped = 0; >>>> u64 size = unmap->size; >>>> >>>> static struct rb_node *vfio_find_dma_first_node( >>>> struct vfio_iommu *iommu, dma_addr_t start, size_t size) >>>> ==> >>>> static struct rb_node *vfio_find_dma_first_node( >>>> struct vfio_iommu *iommu, dma_addr_t start, u64 size) >>>> >>>> And maybe use dma_addr_t instead of u64 in the above (which is 64 bits for >>>> CONFIG_X86_PAE). >>>> >>>> However, there are other places in the existing code that need tweaking >>>> to be safe for PAE, the vfio_find_dma() size arg for one. >>> >>> Yes, it looks like the IOMMU aperture checking using vfio_find_dma() >>> could have issues where dma_addr_t > size_t. Do you want to propose a >>> patch? Thanks, >>> >>> Alex >>> >>>>> 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@lists.01.org >>>>>> >>>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-23 21:52 ` Steven Sistare @ 2021-02-23 23:58 ` Steven Sistare 2021-02-24 22:55 ` Alex Williamson 0 siblings, 1 reply; 15+ messages in thread From: Steven Sistare @ 2021-02-23 23:58 UTC (permalink / raw) To: Alex Williamson Cc: Dan Carpenter, kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck 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@oracle.com> wrote: >> >>> On 2/23/2021 12:45 PM, Alex Williamson wrote: >>>> On Tue, 23 Feb 2021 08:56:36 -0500 >>>> Steven Sistare <steven.sistare@oracle.com> wrote: >>>> >>>>> On 2/22/2021 6:17 PM, Alex Williamson wrote: >>>>>> On Mon, 22 Feb 2021 15:51:45 -0700 >>>>>> Alex Williamson <alex.williamson@redhat.com> wrote: >>>>>> >>>>>>> On Mon, 22 Feb 2021 17:10:43 +0300 >>>>>>> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> >>>>>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>>>>>>> >>>>>>>> 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. - Steve >> unmap-all logic only looks for a first entry to unmap in the >> [0..SIZE_MAX] range. If an entry happens to exist there, we'll unmap >> everything, but the user would have no requirement to have a mapping >> within that range, their first mapping could exist at iova = (SIZE_MAX >> + 1). So unmap-all would effectively need a special case to use >> rb_first(), which mostly negates the reason we added >> vfio_find_dma_first_node(). Right? Thanks, >> >> Alex >> >> >>>>> For unmap, these fixes should suffice, and I would rather do this than >>>>> disable the unmap-all flag for a corner case: >>>>> >>>>> vfio_dma_do_unmap() >>>>> size_t unmapped = 0; >>>>> unsigned long size = unmap->size; >>>>> ==> >>>>> u64 unmapped = 0; >>>>> u64 size = unmap->size; >>>>> >>>>> static struct rb_node *vfio_find_dma_first_node( >>>>> struct vfio_iommu *iommu, dma_addr_t start, size_t size) >>>>> ==> >>>>> static struct rb_node *vfio_find_dma_first_node( >>>>> struct vfio_iommu *iommu, dma_addr_t start, u64 size) >>>>> >>>>> And maybe use dma_addr_t instead of u64 in the above (which is 64 bits for >>>>> CONFIG_X86_PAE). >>>>> >>>>> However, there are other places in the existing code that need tweaking >>>>> to be safe for PAE, the vfio_find_dma() size arg for one. >>>> >>>> Yes, it looks like the IOMMU aperture checking using vfio_find_dma() >>>> could have issues where dma_addr_t > size_t. Do you want to propose a >>>> patch? Thanks, >>>> >>>> Alex >>>> >>>>>> 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@lists.01.org >>>>>>> >>>>>> >>>>> >>>> >>> >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-23 23:58 ` Steven Sistare @ 2021-02-24 22:55 ` Alex Williamson 2021-02-25 15:25 ` Steven Sistare 0 siblings, 1 reply; 15+ messages in thread From: Alex Williamson @ 2021-02-24 22:55 UTC (permalink / raw) To: Steven Sistare Cc: Dan Carpenter, kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On Tue, 23 Feb 2021 18:58:18 -0500 Steven Sistare <steven.sistare@oracle.com> 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@oracle.com> wrote: > >> > >>> On 2/23/2021 12:45 PM, Alex Williamson wrote: > >>>> On Tue, 23 Feb 2021 08:56:36 -0500 > >>>> Steven Sistare <steven.sistare@oracle.com> wrote: > >>>> > >>>>> On 2/22/2021 6:17 PM, Alex Williamson wrote: > >>>>>> On Mon, 22 Feb 2021 15:51:45 -0700 > >>>>>> Alex Williamson <alex.williamson@redhat.com> wrote: > >>>>>> > >>>>>>> On Mon, 22 Feb 2021 17:10:43 +0300 > >>>>>>> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> > >>>>>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >>>>>>>> > >>>>>>>> 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, Alex ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-24 22:55 ` Alex Williamson @ 2021-02-25 15:25 ` Steven Sistare 2021-02-25 18:00 ` Alex Williamson 0 siblings, 1 reply; 15+ messages in thread From: Steven Sistare @ 2021-02-25 15:25 UTC (permalink / raw) To: Alex Williamson Cc: Dan Carpenter, kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On 2/24/2021 5:55 PM, Alex Williamson wrote: > On Tue, 23 Feb 2021 18:58:18 -0500 > Steven Sistare <steven.sistare@oracle.com> 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@oracle.com> wrote: >>>> >>>>> On 2/23/2021 12:45 PM, Alex Williamson wrote: >>>>>> On Tue, 23 Feb 2021 08:56:36 -0500 >>>>>> Steven Sistare <steven.sistare@oracle.com> wrote: >>>>>> >>>>>>> On 2/22/2021 6:17 PM, Alex Williamson wrote: >>>>>>>> On Mon, 22 Feb 2021 15:51:45 -0700 >>>>>>>> Alex Williamson <alex.williamson@redhat.com> wrote: >>>>>>>> >>>>>>>>> On Mon, 22 Feb 2021 17:10:43 +0300 >>>>>>>>> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> >>>>>>>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>>>>>>>>> >>>>>>>>>> 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))) { ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-25 15:25 ` Steven Sistare @ 2021-02-25 18:00 ` Alex Williamson 2021-02-25 18:52 ` Steven Sistare 0 siblings, 1 reply; 15+ messages in thread From: Alex Williamson @ 2021-02-25 18:00 UTC (permalink / raw) To: Steven Sistare Cc: Dan Carpenter, kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On Thu, 25 Feb 2021 10:25:16 -0500 Steven Sistare <steven.sistare@oracle.com> wrote: > On 2/24/2021 5:55 PM, Alex Williamson wrote: > > On Tue, 23 Feb 2021 18:58:18 -0500 > > Steven Sistare <steven.sistare@oracle.com> 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@oracle.com> wrote: > >>>> > >>>>> On 2/23/2021 12:45 PM, Alex Williamson wrote: > >>>>>> On Tue, 23 Feb 2021 08:56:36 -0500 > >>>>>> Steven Sistare <steven.sistare@oracle.com> wrote: > >>>>>> > >>>>>>> On 2/22/2021 6:17 PM, Alex Williamson wrote: > >>>>>>>> On Mon, 22 Feb 2021 15:51:45 -0700 > >>>>>>>> Alex Williamson <alex.williamson@redhat.com> wrote: > >>>>>>>> > >>>>>>>>> On Mon, 22 Feb 2021 17:10:43 +0300 > >>>>>>>>> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> > >>>>>>>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >>>>>>>>>> > >>>>>>>>>> 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, Alex > 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))) { > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-25 18:00 ` Alex Williamson @ 2021-02-25 18:52 ` Steven Sistare 2021-02-25 19:12 ` Alex Williamson 0 siblings, 1 reply; 15+ messages in thread From: Steven Sistare @ 2021-02-25 18:52 UTC (permalink / raw) To: Alex Williamson Cc: Dan Carpenter, kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck [-- Attachment #1: Type: text/plain, Size: 7327 bytes --] On 2/25/2021 1:00 PM, Alex Williamson wrote: > On Thu, 25 Feb 2021 10:25:16 -0500 > Steven Sistare <steven.sistare@oracle.com> wrote: > >> On 2/24/2021 5:55 PM, Alex Williamson wrote: >>> On Tue, 23 Feb 2021 18:58:18 -0500 >>> Steven Sistare <steven.sistare@oracle.com> 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@oracle.com> wrote: >>>>>> >>>>>>> On 2/23/2021 12:45 PM, Alex Williamson wrote: >>>>>>>> On Tue, 23 Feb 2021 08:56:36 -0500 >>>>>>>> Steven Sistare <steven.sistare@oracle.com> wrote: >>>>>>>> >>>>>>>>> On 2/22/2021 6:17 PM, Alex Williamson wrote: >>>>>>>>>> On Mon, 22 Feb 2021 15:51:45 -0700 >>>>>>>>>> Alex Williamson <alex.williamson@redhat.com> wrote: >>>>>>>>>> >>>>>>>>>>> On Mon, 22 Feb 2021 17:10:43 +0300 >>>>>>>>>>> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> >>>>>>>>>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>>>>>>>>>>> >>>>>>>>>>>> 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 [-- Attachment #2: 0001-vfio-type1-fix-unmap-all-on-ILP32.patch --] [-- Type: text/plain, Size: 2536 bytes --] From ba86ef0e273f5bccb54b984ea305beb2857134c1 Mon Sep 17 00:00:00 2001 From: Steve Sistare <steven.sistare@oracle.com> Date: Thu, 25 Feb 2021 10:20:04 -0800 Subject: [PATCH] vfio/type1: fix unmap all on ILP32 Some ILP32 architectures support mapping a 32-bit vaddr within a 64-bit iova space. The unmap-all code uses 32-bit SIZE_MAX as an upper bound on the extent of the mappings within iova space, so mappings above 4G cannot be found and unmapped. Use U64_MAX instead, and use u64 for size variables. This also fixes a static analysis bug found by the kernel test robot running smatch for ILP32. Fixes: 0f53afa ("vfio/type1: unmap cleanup") Fixes: c196509 ("vfio/type1: implement unmap all") Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> Reviewed-by: Alex Williamson <alex.williamson@redhat.com> --- drivers/vfio/vfio_iommu_type1.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) 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))) { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-25 18:52 ` Steven Sistare @ 2021-02-25 19:12 ` Alex Williamson 0 siblings, 0 replies; 15+ messages in thread From: Alex Williamson @ 2021-02-25 19:12 UTC (permalink / raw) To: Steven Sistare Cc: Dan Carpenter, kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On Thu, 25 Feb 2021 13:52:32 -0500 Steven Sistare <steven.sistare@oracle.com> wrote: > On 2/25/2021 1:00 PM, Alex Williamson wrote: > > On Thu, 25 Feb 2021 10:25:16 -0500 > > Steven Sistare <steven.sistare@oracle.com> wrote: > > > >> On 2/24/2021 5:55 PM, Alex Williamson wrote: > >>> On Tue, 23 Feb 2021 18:58:18 -0500 > >>> Steven Sistare <steven.sistare@oracle.com> 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@oracle.com> wrote: > >>>>>> > >>>>>>> On 2/23/2021 12:45 PM, Alex Williamson wrote: > >>>>>>>> On Tue, 23 Feb 2021 08:56:36 -0500 > >>>>>>>> Steven Sistare <steven.sistare@oracle.com> wrote: > >>>>>>>> > >>>>>>>>> On 2/22/2021 6:17 PM, Alex Williamson wrote: > >>>>>>>>>> On Mon, 22 Feb 2021 15:51:45 -0700 > >>>>>>>>>> Alex Williamson <alex.williamson@redhat.com> wrote: > >>>>>>>>>> > >>>>>>>>>>> On Mon, 22 Feb 2021 17:10:43 +0300 > >>>>>>>>>>> Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> > >>>>>>>>>>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > >>>>>>>>>>>> > >>>>>>>>>>>> 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' 2021-02-22 22:51 ` Alex Williamson 2021-02-22 23:17 ` Alex Williamson @ 2021-02-23 13:20 ` Steven Sistare 1 sibling, 0 replies; 15+ messages in thread From: Steven Sistare @ 2021-02-23 13:20 UTC (permalink / raw) To: Alex Williamson, Dan Carpenter Cc: kbuild, lkp, kbuild-all, Linux Memory Management List, Cornelia Huck On 2/22/2021 5:51 PM, Alex Williamson wrote: > On Mon, 22 Feb 2021 17:10:43 +0300 > Dan Carpenter <dan.carpenter@oracle.com> 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@intel.com> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> >> 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@lists.01.org > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-02-25 19:12 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-22 14:10 [kbuild] [linux-next:master 6931/12022] drivers/vfio/vfio_iommu_type1.c:1093 vfio_dma_do_unmap() warn: impossible condition '(size > (~0)) => (0-u32max > u32max)' Dan Carpenter 2021-02-22 22:51 ` Alex Williamson 2021-02-22 23:17 ` Alex Williamson 2021-02-23 13:56 ` Steven Sistare 2021-02-23 17:45 ` Alex Williamson 2021-02-23 20:37 ` Steven Sistare 2021-02-23 21:10 ` Alex Williamson 2021-02-23 21:52 ` Steven Sistare 2021-02-23 23:58 ` Steven Sistare 2021-02-24 22:55 ` Alex Williamson 2021-02-25 15:25 ` Steven Sistare 2021-02-25 18:00 ` Alex Williamson 2021-02-25 18:52 ` Steven Sistare 2021-02-25 19:12 ` Alex Williamson 2021-02-23 13:20 ` Steven Sistare
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).