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

[Dan Carpenter <error27@gmail.com>]

On Tue, Nov 15, 2022 at 04:29:20PM -0400, Jason Gunthorpe wrote:
> 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?

No.  I've written something like five different integer overflow checks.
One is pretty decent but the problem is a hard nut to crack.  This
heuristic here is not useful enough to the point where we'd want to
silence false positives.

regards,
dan carpenter