Adopt check_*_overflow functions to clearly express overflow check
intent.
Signed-off-by: Alex Mastro <amastro@fb.com>
---
drivers/vfio/vfio_iommu_type1.c | 54 ++++++++++++++++++++++++++++++++---------
1 file changed, 43 insertions(+), 11 deletions(-)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index f8d68fe77b41..b510ef3f397b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -37,6 +37,7 @@
#include <linux/vfio.h>
#include <linux/workqueue.h>
#include <linux/notifier.h>
+#include <linux/overflow.h>
#include "vfio.h"
#define DRIVER_VERSION "0.2"
@@ -825,14 +826,25 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
unsigned long remote_vaddr;
struct vfio_dma *dma;
bool do_accounting;
+ dma_addr_t iova_end;
+ size_t iova_size;
- if (!iommu || !pages)
+ if (!iommu || !pages || npage < 0)
return -EINVAL;
/* Supported for v2 version only */
if (!iommu->v2)
return -EACCES;
+ if (npage == 0)
+ return 0;
+
+ if (check_mul_overflow(npage, PAGE_SIZE, &iova_size))
+ return -EINVAL;
+
+ if (check_add_overflow(user_iova, iova_size - 1, &iova_end))
+ return -EINVAL;
+
mutex_lock(&iommu->lock);
if (WARN_ONCE(iommu->vaddr_invalid_count,
@@ -938,12 +950,23 @@ static void vfio_iommu_type1_unpin_pages(void *iommu_data,
{
struct vfio_iommu *iommu = iommu_data;
bool do_accounting;
+ dma_addr_t iova_end;
+ size_t iova_size;
int i;
/* Supported for v2 version only */
if (WARN_ON(!iommu->v2))
return;
+ if (WARN_ON(npage < 0) || npage == 0)
+ return;
+
+ if (WARN_ON(check_mul_overflow(npage, PAGE_SIZE, &iova_size)))
+ return;
+
+ if (WARN_ON(check_add_overflow(user_iova, iova_size - 1, &iova_end)))
+ return;
+
mutex_lock(&iommu->lock);
do_accounting = list_empty(&iommu->domain_list);
@@ -1304,6 +1327,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;
+ dma_addr_t iova_end;
u64 size = unmap->size;
bool unmap_all = unmap->flags & VFIO_DMA_UNMAP_FLAG_ALL;
bool invalidate_vaddr = unmap->flags & VFIO_DMA_UNMAP_FLAG_VADDR;
@@ -1328,7 +1352,8 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
goto unlock;
size = U64_MAX;
} else if (!size || size & (pgsize - 1) ||
- iova + size - 1 < iova || size > SIZE_MAX) {
+ check_add_overflow(iova, size - 1, &iova_end) ||
+ size > SIZE_MAX) {
goto unlock;
}
@@ -1376,7 +1401,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
if (dma && dma->iova != iova)
goto unlock;
- dma = vfio_find_dma(iommu, iova + size - 1, 0);
+ dma = vfio_find_dma(iommu, iova_end, 0);
if (dma && dma->iova + dma->size != iova + size)
goto unlock;
}
@@ -1578,7 +1603,9 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
{
bool set_vaddr = map->flags & VFIO_DMA_MAP_FLAG_VADDR;
dma_addr_t iova = map->iova;
+ dma_addr_t iova_end;
unsigned long vaddr = map->vaddr;
+ unsigned long vaddr_end;
size_t size = map->size;
int ret = 0, prot = 0;
size_t pgsize;
@@ -1588,6 +1615,12 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
if (map->size != size || map->vaddr != vaddr || map->iova != iova)
return -EINVAL;
+ if (check_add_overflow(iova, size - 1, &iova_end))
+ return -EINVAL;
+
+ if (check_add_overflow(vaddr, size - 1, &vaddr_end))
+ return -EINVAL;
+
/* READ/WRITE from device perspective */
if (map->flags & VFIO_DMA_MAP_FLAG_WRITE)
prot |= IOMMU_WRITE;
@@ -1608,12 +1641,6 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
goto out_unlock;
}
- /* Don't allow IOVA or virtual address wrap */
- if (iova + size - 1 < iova || vaddr + size - 1 < vaddr) {
- ret = -EINVAL;
- goto out_unlock;
- }
-
dma = vfio_find_dma(iommu, iova, size);
if (set_vaddr) {
if (!dma) {
@@ -1640,7 +1667,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
goto out_unlock;
}
- if (!vfio_iommu_iova_dma_valid(iommu, iova, iova + size - 1)) {
+ if (!vfio_iommu_iova_dma_valid(iommu, iova, iova_end)) {
ret = -EINVAL;
goto out_unlock;
}
@@ -2908,6 +2935,7 @@ static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu,
unsigned long pgshift;
size_t data_size = dirty.argsz - minsz;
size_t iommu_pgsize;
+ dma_addr_t range_end;
if (!data_size || data_size < sizeof(range))
return -EINVAL;
@@ -2916,8 +2944,12 @@ static int vfio_iommu_type1_dirty_pages(struct vfio_iommu *iommu,
sizeof(range)))
return -EFAULT;
- if (range.iova + range.size < range.iova)
+ if (range.size == 0)
+ return 0;
+
+ if (check_add_overflow(range.iova, range.size - 1, &range_end))
return -EINVAL;
+
if (!access_ok((void __user *)range.bitmap.data,
range.bitmap.size))
return -EINVAL;
--
2.47.3On Tue, Oct 07, 2025 at 09:08:46PM -0700, Alex Mastro wrote: > Adopt check_*_overflow functions to clearly express overflow check > intent. > > Signed-off-by: Alex Mastro <amastro@fb.com> > --- > drivers/vfio/vfio_iommu_type1.c | 54 ++++++++++++++++++++++++++++++++--------- > 1 file changed, 43 insertions(+), 11 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index f8d68fe77b41..b510ef3f397b 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -37,6 +37,7 @@ > #include <linux/vfio.h> > #include <linux/workqueue.h> > #include <linux/notifier.h> > +#include <linux/overflow.h> > #include "vfio.h" > > #define DRIVER_VERSION "0.2" > @@ -825,14 +826,25 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data, > unsigned long remote_vaddr; > struct vfio_dma *dma; > bool do_accounting; > + dma_addr_t iova_end; > + size_t iova_size; > > - if (!iommu || !pages) > + if (!iommu || !pages || npage < 0) > return -EINVAL; > > /* Supported for v2 version only */ > if (!iommu->v2) > return -EACCES; > > + if (npage == 0) > + return 0; > + > + if (check_mul_overflow(npage, PAGE_SIZE, &iova_size)) > + return -EINVAL; -EOVERFLOW and everywhere else > + > + if (check_add_overflow(user_iova, iova_size - 1, &iova_end)) > + return -EINVAL; Let's be consistent with iommufd/etc, 'end' is start+size 'last' is start+size-1 Otherwise it is super confusing :( Jason
On Wed, Oct 08, 2025 at 09:19:30AM -0300, Jason Gunthorpe wrote: > On Tue, Oct 07, 2025 at 09:08:46PM -0700, Alex Mastro wrote: > > + if (check_mul_overflow(npage, PAGE_SIZE, &iova_size)) > > + return -EINVAL; > > -EOVERFLOW and everywhere else > > > + > > + if (check_add_overflow(user_iova, iova_size - 1, &iova_end)) > > + return -EINVAL; > > Let's be consistent with iommufd/etc, 'end' is start+size 'last' is start+size-1 > > Otherwise it is super confusing :( Both suggestions SGTM.
On Wed, Oct 08, 2025 at 08:39:21AM -0700, Alex Mastro wrote:
> On Wed, Oct 08, 2025 at 09:19:30AM -0300, Jason Gunthorpe wrote:
> > On Tue, Oct 07, 2025 at 09:08:46PM -0700, Alex Mastro wrote:
> > > + if (check_add_overflow(user_iova, iova_size - 1, &iova_end))
> > > + return -EINVAL;
> >
> > Let's be consistent with iommufd/etc, 'end' is start+size 'last' is start+size-1
> >
> > Otherwise it is super confusing :(
>
>
> Both suggestions SGTM.
I'm not sure about the latter anymore. There's somewhat pervasive precedent for
using 'end' as the inclusive limit in vfio_iommu_type1.c. I am all for making
things less confusing. I don't think I can introduce 'end' 'last' convention
without preparing the existing code first.
Thoughts? Spend another commit renaming this to 'last'? Tolerate inconsistency
between vfio and iommufd?
116 struct vfio_iova {
117 struct list_head list;
118 dma_addr_t start;
119 dma_addr_t end;
120 };
...
2037 end = resv->start + resv->length - 1;
2038
2039 list_for_each_entry_safe(n, next, iova, list) {
2040 int ret = 0;
2041
2042 /* No overlap */
2043 if (start > n->end || end < n->start)
2044 continue;
...
2052 if (start > n->start)
2053 ret = vfio_iommu_iova_insert(&n->list, n->start,
2054 start - 1);
2055 if (!ret && end < n->end)
2056 ret = vfio_iommu_iova_insert(&n->list, end + 1,
2057 n->end);
On Wed, Oct 08, 2025 at 03:19:07PM -0700, Alex Mastro wrote: > On Wed, Oct 08, 2025 at 08:39:21AM -0700, Alex Mastro wrote: > > On Wed, Oct 08, 2025 at 09:19:30AM -0300, Jason Gunthorpe wrote: > > > On Tue, Oct 07, 2025 at 09:08:46PM -0700, Alex Mastro wrote: > > > > + if (check_add_overflow(user_iova, iova_size - 1, &iova_end)) > > > > + return -EINVAL; > > > > > > Let's be consistent with iommufd/etc, 'end' is start+size 'last' is start+size-1 > > > > > > Otherwise it is super confusing :( > > > > > > Both suggestions SGTM. > > I'm not sure about the latter anymore. There's somewhat pervasive precedent for > using 'end' as the inclusive limit in vfio_iommu_type1.c. I am all for making > things less confusing. I don't think I can introduce 'end' 'last' convention > without preparing the existing code first. > > Thoughts? Spend another commit renaming this to 'last'? Tolerate inconsistency > between vfio and iommufd? IDK, if it is actually internally consistent and not using end interchangably then it is probably Ok to keep doing it. If it is already inconsistent then use last for new code and leave the old as is? Jason
On Wed, Oct 08, 2025 at 10:15:35PM -0300, Jason Gunthorpe wrote: > On Wed, Oct 08, 2025 at 03:19:07PM -0700, Alex Mastro wrote: > > On Wed, Oct 08, 2025 at 08:39:21AM -0700, Alex Mastro wrote: > > > On Wed, Oct 08, 2025 at 09:19:30AM -0300, Jason Gunthorpe wrote: > > > > On Tue, Oct 07, 2025 at 09:08:46PM -0700, Alex Mastro wrote: > > > > > + if (check_add_overflow(user_iova, iova_size - 1, &iova_end)) > > > > > + return -EINVAL; > > > > > > > > Let's be consistent with iommufd/etc, 'end' is start+size 'last' is start+size-1 > > > > > > > > Otherwise it is super confusing :( > > > > > > > > > Both suggestions SGTM. > > > > I'm not sure about the latter anymore. There's somewhat pervasive precedent for > > using 'end' as the inclusive limit in vfio_iommu_type1.c. I am all for making > > things less confusing. I don't think I can introduce 'end' 'last' convention > > without preparing the existing code first. > > > > Thoughts? Spend another commit renaming this to 'last'? Tolerate inconsistency > > between vfio and iommufd? > > IDK, if it is actually internally consistent and not using end > interchangably then it is probably Ok to keep doing it. If it is > already inconsistent then use last for new code and leave the old as > is? The only references to 'last' are for elements in a list, tree, unrelated to iova, and 'end' refers to (iova+size-1) for all cases that I saw. For the sake of internal consistency, I'll keep 'end', and after this series, if it's worth it, we can take a pass to unify the terminology between iommufd and vfio.
© 2016 - 2026 Red Hat, Inc.