On Thu, Oct 12, 2017 at 10:32:41AM -0600, Alex Williamson wrote:
> Hi Dan,
>
> On Thu, 12 Oct 2017 15:40:36 +0300
> Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> > Hello Alex Williamson,
> >
> > The patch 166fd7d94afd: "vfio: hugepage support for vfio_iommu_type1"
> > from Jun 21, 2013, leads to the following static checker warning:
> >
> > drivers/vfio/vfio_iommu_type1.c:819 vfio_dma_do_unmap()
> > warn: overflowed symbol reused: 'unmap->size'
> >
> > drivers/vfio/vfio_iommu_type1.c
> > 756 static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
> > 757 struct vfio_iommu_type1_dma_unmap *unmap)
> > 758 {
> > 759 uint64_t mask;
> > 760 struct vfio_dma *dma, *dma_last = NULL;
> > 761 size_t unmapped = 0;
> > 762 int ret = 0, retries = 0;
> > 763
> > 764 mask = ((uint64_t)1 << __ffs(vfio_pgsize_bitmap(iommu))) - 1;
> > 765
> > 766 if (unmap->iova & mask)
> > 767 return -EINVAL;
> > 768 if (!unmap->size || unmap->size & mask)
> > 769 return -EINVAL;
> >
> > unmap is controlled by the user from the ioctl. ->iova and ->size are
> > type u64. It would be simple enough to add an integer overflow check
> > here, but I'd be tempted to do something like:
> >
> > if (unmap->size + unmap->iova < unmap->size ||
> > unmap->size + unmap->iova > SIZE_MAX)
> > return -EINVAL;
> >
> > Because below we are passing it to vfio_find_dma() which only takes a
> > size_t and not a u64.
>
> Ok, but why test unmap->iova + unmap->size relative to SIZE_MAX rather
> than just unmap->size since we use a dma_addr_t for the iova in
> vfio_find_dma()?
Oh, yeah, you're right. I misread the code a bit. I'll send a check
for that then.
regards,
dan carpenter
