Hi Alex,
> Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Thursday, July 9, 2020 3:55 AM
>
> On Wed, 8 Jul 2020 08:16:16 +0000
> "Liu, Yi L" <yi.l.liu@xxxxxxxxx> wrote:
>
> > Hi Alex,
> >
> > > From: Liu, Yi L < yi.l.liu@xxxxxxxxx>
> > > Sent: Friday, July 3, 2020 2:28 PM
> > >
> > > Hi Alex,
> > >
> > > > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > > > Sent: Friday, July 3, 2020 5:19 AM
> > > >
> > > > On Wed, 24 Jun 2020 01:55:19 -0700
> > > > Liu Yi L <yi.l.liu@xxxxxxxxx> wrote:
> > > >
> > > > > This patch allows user space to request PASID allocation/free, e.g.
> > > > > when serving the request from the guest.
> > > > >
> > > > > PASIDs that are not freed by userspace are automatically freed when
> > > > > the IOASID set is destroyed when process exits.
> > [...]
> > > > > +static int vfio_iommu_type1_pasid_request(struct vfio_iommu *iommu,
> > > > > + unsigned long arg)
> > > > > +{
> > > > > + struct vfio_iommu_type1_pasid_request req;
> > > > > + unsigned long minsz;
> > > > > +
> > > > > + minsz = offsetofend(struct vfio_iommu_type1_pasid_request,
> range);
> > > > > +
> > > > > + if (copy_from_user(&req, (void __user *)arg, minsz))
> > > > > + return -EFAULT;
> > > > > +
> > > > > + if (req.argsz < minsz || (req.flags &
> ~VFIO_PASID_REQUEST_MASK))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (req.range.min > req.range.max)
> > > >
> > > > Is it exploitable that a user can spin the kernel for a long time in
> > > > the case of a free by calling this with [0, MAX_UINT] regardless of their
> actual
> > > allocations?
> > >
> > > IOASID can ensure that user can only free the PASIDs allocated to the user.
> but
> > > it's true, kernel needs to loop all the PASIDs within the range provided
> > > by user.
> it
> > > may take a long time. is there anything we can do? one thing may limit the
> range
> > > provided by user?
> >
> > thought about it more, we have per-VM pasid quota (say 1000), so even if
> > user passed down [0, MAX_UNIT], kernel will only loop the 1000 pasids at
> > most. do you think we still need to do something on it?
>
> How do you figure that? vfio_iommu_type1_pasid_request() accepts the
> user's min/max so long as (max > min) and passes that to
> vfio_iommu_type1_pasid_free(), then to vfio_pasid_free_range() which
> loops as:
>
> ioasid_t pasid = min;
> for (; pasid <= max; pasid++)
> ioasid_free(pasid);
>
> A user might only be able to allocate 1000 pasids, but apparently they
> can ask to free all they want.
>
> It's also not obvious to me that calling ioasid_free() is only allowing
> the user to free their own passid. Does it? It would be a pretty
> gaping hole if a user could free arbitrary pasids. A r-b tree of
> passids might help both for security and to bound spinning in a loop.
oh, yes. BTW. instead of r-b tree in VFIO, maybe we can add an ioasid_set
parameter for ioasid_free(), thus to prevent the user from freeing PASIDs
that doesn't belong to it. I remember Jacob mentioned it before.
@Jacob, is it still in your plan?
Regards,
Yi Liu
> Thanks,
>
> Alex
