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()?
> I don't think it actually matters? The heuristic here is that we look
> for user triggered integer overflows and then we re-use the variables
> again. There are too many integer overflows (thousands) which aren't
> harmful and I haven't yet figured out an easy way to filter for just
> the harmful ones...
Right, overflows here seem like they could only hurt the user in
management of their own iova address space, not the kernel integrity,
but if adding the above check provides a sanity check for the user and
makes debugging a little easier, I'm for it. Thanks,
Alex
