Re: [bug report] iommufd: vfio container FD ioctl compatibility

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Nov 15, 2022 at 03:45:00PM +0300, Dan Carpenter wrote:
> [ Resending two weeks of email because mutt + msmtp has been silently
>   eating my outgoing mail instead of delivering it.  *sigh* -dan ]
> 
> Hello Jason Gunthorpe,
> 
> The patch 32c328dc9b73: "iommufd: vfio container FD ioctl
> compatibility" from Dec 15, 2021, leads to the following Smatch
> static checker warning:
> 
> 	drivers/iommu/iommufd/vfio_compat.c:174 iommufd_vfio_unmap_dma()
> 	warn: potential integer overflow from user (local copy) 'unmap.iova + unmap.size'
> 
> drivers/iommu/iommufd/vfio_compat.c
>     140 static int iommufd_vfio_unmap_dma(struct iommufd_ctx *ictx, unsigned int cmd,
>     141                                   void __user *arg)
>     142 {
>     143         size_t minsz = offsetofend(struct vfio_iommu_type1_dma_unmap, size);
>     144         /*
>     145          * VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP is obsoleted by the new
>     146          * dirty tracking direction:
>     147          *  https://lore.kernel.org/kvm/20220731125503.142683-1-yishaih@nvidia.com/
>     148          *  https://lore.kernel.org/kvm/20220428210933.3583-1-joao.m.martins@oracle.com/
>     149          */
>     150         u32 supported_flags = VFIO_DMA_UNMAP_FLAG_ALL;
>     151         struct vfio_iommu_type1_dma_unmap unmap;
>     152         struct iommufd_ioas *ioas;
>     153         unsigned long unmapped;
>     154         int rc;
>     155 
>     156         if (copy_from_user(&unmap, arg, minsz))
>     157                 return -EFAULT;
>     158 
>     159         if (unmap.argsz < minsz || unmap.flags & ~supported_flags)
>     160                 return -EINVAL;
>     161 
>     162         ioas = get_compat_ioas(ictx);
>     163         if (IS_ERR(ioas))
>     164                 return PTR_ERR(ioas);
>     165 
>     166         if (unmap.flags & VFIO_DMA_UNMAP_FLAG_ALL) {
>     167                 if (unmap.iova != 0 || unmap.size != 0) {
>     168                         rc = -EINVAL;
>     169                         goto err_put;
>     170                 }
>     171                 rc = iopt_unmap_all(&ioas->iopt, &unmapped);
>     172         } else {
>     173                 if (READ_ONCE(ioas->iopt.disable_large_pages)) {
> --> 174                         unsigned long iovas[] = { unmap.iova + unmap.size - 1,
> 
> The unmap.iova + unmap.size addition can have an integer overflow.  It's
> unclear to me if this is caught later or if it has any negative
> implications.

There are no negative implications to the kernel, which is why I left
it unchecked here.

Do you think we should have the check for the sake of static tools?

Jason