Re: [bug report] vfio: hugepage support for vfio_iommu_type1

[Alex Williamson <alex.williamson@redhat.com>]

Hi Dan,

On Thu, 12 Oct 2017 15:40:36 +0300
Dan Carpenter <dan.carpenter@oracle.com> 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