RE: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
> From: Alex Williamson
> Sent: Friday, July 10, 2020 8:55 PM
>
> On Fri, 10 Jul 2020 05:39:57 +
> "Liu, Yi L" wrote:
>
> > Hi Alex,
> >
> > > From: Alex Williamson
> > > Sent: Thursday, July 9, 2020 10:28 PM
> > >
> > > On Thu, 9 Jul 2020 07:16:31 +
> > > "Liu, Yi L" wrote:
> > >
> > > > Hi Alex,
> > > >
> > > > After more thinking, looks like adding a r-b tree is still not enough to
> > > > solve the potential problem for free a range of PASID in one ioctl. If
> > > > caller gives [0, MAX_UNIT] in the free request, kernel anyhow should
> > > > loop all the PASIDs and search in the r-b tree. Even VFIO can track the
> > > > smallest/largest allocated PASID, and limit the free range to an
> > > > accurate
> > > > range, it is still no efficient. For example, user has allocated two
> > > > PASIDs
> > > > ( 1 and 999), and user gives the [0, MAX_UNIT] range in free request.
> > > > VFIO
> > > > will limit the free range to be [1, 999], but still needs to loop PASID
> > > > 1 -
> > > > 999, and search in r-b tree.
> > >
> > > That sounds like a poor tree implementation. Look at vfio_find_dma()
> > > for instance, it returns a node within the specified range. If the
> > > tree has two nodes within the specified range we should never need to
> > > call a search function like vfio_find_dma() more than three times. We
> > > call it once, get the first node, remove it. Call it again, get the
> > > other node, remove it. Call a third time, find no matches, we're done.
> > > So such an implementation limits searches to N+1 where N is the number
> > > of nodes within the range.
> >
> > I see. When getting a free range from user. Use the range to find suited
> > PASIDs in the r-b tree. For the example I mentioned, if giving [0,
> > MAX_UNIT],
> > will find two nodes. If giving [0, 100] range, then only one node will be
> > found. But even though, it still take some time if the user holds a bunch
> > of PASIDs and user gives a big free range.
>
>
> But that time is bounded. The complexity of the tree and maximum
> number of operations on the tree are bounded by the number of nodes,
> which is bound by the user's pasid quota. Thanks,
yes, let me try it. thanks. :-)
Regards,
Yi Liu
> Alex
>
> > > > So I'm wondering can we fall back to prior proposal which only free one
> > > > PASID for a free request. how about your opinion?
> > >
> > > Doesn't it still seem like it would be a useful user interface to have
> > > a mechanism to free all pasids, by calling with exactly [0, MAX_UINT]?
> > > I'm not sure if there's another use case for this given than the user
> > > doesn't have strict control of the pasid values they get. Thanks,
> >
> > I don't have such use case neither. perhaps we may allow it in future by
> > adding flag. but if it's still useful, I may try with your suggestion. :-)
> >
> > Regards,
> > Yi Liu
> >
> > > Alex
> > >
> > > > > From: Liu, Yi L
> > > > > Sent: Thursday, July 9, 2020 10:26 AM
> > > > >
> > > > > Hi Kevin,
> > > > >
> > > > > > From: Tian, Kevin
> > > > > > Sent: Thursday, July 9, 2020 10:18 AM
> > > > > >
> > > > > > > From: Liu, Yi L
> > > > > > > Sent: Thursday, July 9, 2020 10:08 AM
> > > > > > >
> > > > > > > Hi Kevin,
> > > > > > >
> > > > > > > > From: Tian, Kevin
> > > > > > > > Sent: Thursday, July 9, 2020 9:57 AM
> > > > > > > >
> > > > > > > > > From: Liu, Yi L
> > > > > > > > > Sent: Thursday, July 9, 2020 8:32 AM
> > > > > > > > >
> > > > > > > > > Hi Alex,
> > > > > > > > >
> > > > > > > > > > Alex Williamson
> > > > > > > > > > Sent: Thursday, July 9, 2020 3:55 AM
> > > > > > > > > >
> > > > > > > > > > On Wed, 8 Jul 2020 08:16:16 + "Liu, Yi L"
> > > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > > Hi Alex,
> > > > > > > > > > >
> > > > > > > > > > > > From: Liu, Yi L < yi.l@intel.com>
> > > > > > > > > > > > Sent: Friday, July 3, 2020 2:28 PM
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Alex,
> > > > > > > > > > > >
> > > > > > > > > > > > > From: Alex Williamson
> > > > > > > > > > > > > Sent: Friday, July 3, 2020 5:19 AM
> > > > > > > > > > > > >
> > > > > > > > > > > > > On Wed, 24 Jun 2020 01:55:19 -0700 Liu Yi L
> > > > > > > > > > > > > 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) {
> > > > > > > > > > > > > > +
Re: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
On Fri, 10 Jul 2020 05:39:57 +
"Liu, Yi L" wrote:
> Hi Alex,
>
> > From: Alex Williamson
> > Sent: Thursday, July 9, 2020 10:28 PM
> >
> > On Thu, 9 Jul 2020 07:16:31 +
> > "Liu, Yi L" wrote:
> >
> > > Hi Alex,
> > >
> > > After more thinking, looks like adding a r-b tree is still not enough to
> > > solve the potential problem for free a range of PASID in one ioctl. If
> > > caller gives [0, MAX_UNIT] in the free request, kernel anyhow should
> > > loop all the PASIDs and search in the r-b tree. Even VFIO can track the
> > > smallest/largest allocated PASID, and limit the free range to an accurate
> > > range, it is still no efficient. For example, user has allocated two
> > > PASIDs
> > > ( 1 and 999), and user gives the [0, MAX_UNIT] range in free request. VFIO
> > > will limit the free range to be [1, 999], but still needs to loop PASID 1
> > > -
> > > 999, and search in r-b tree.
> >
> > That sounds like a poor tree implementation. Look at vfio_find_dma()
> > for instance, it returns a node within the specified range. If the
> > tree has two nodes within the specified range we should never need to
> > call a search function like vfio_find_dma() more than three times. We
> > call it once, get the first node, remove it. Call it again, get the
> > other node, remove it. Call a third time, find no matches, we're done.
> > So such an implementation limits searches to N+1 where N is the number
> > of nodes within the range.
>
> I see. When getting a free range from user. Use the range to find suited
> PASIDs in the r-b tree. For the example I mentioned, if giving [0, MAX_UNIT],
> will find two nodes. If giving [0, 100] range, then only one node will be
> found. But even though, it still take some time if the user holds a bunch
> of PASIDs and user gives a big free range.
But that time is bounded. The complexity of the tree and maximum
number of operations on the tree are bounded by the number of nodes,
which is bound by the user's pasid quota. Thanks,
Alex
> > > So I'm wondering can we fall back to prior proposal which only free one
> > > PASID for a free request. how about your opinion?
> >
> > Doesn't it still seem like it would be a useful user interface to have
> > a mechanism to free all pasids, by calling with exactly [0, MAX_UINT]?
> > I'm not sure if there's another use case for this given than the user
> > doesn't have strict control of the pasid values they get. Thanks,
>
> I don't have such use case neither. perhaps we may allow it in future by
> adding flag. but if it's still useful, I may try with your suggestion. :-)
>
> Regards,
> Yi Liu
>
> > Alex
> >
> > > > From: Liu, Yi L
> > > > Sent: Thursday, July 9, 2020 10:26 AM
> > > >
> > > > Hi Kevin,
> > > >
> > > > > From: Tian, Kevin
> > > > > Sent: Thursday, July 9, 2020 10:18 AM
> > > > >
> > > > > > From: Liu, Yi L
> > > > > > Sent: Thursday, July 9, 2020 10:08 AM
> > > > > >
> > > > > > Hi Kevin,
> > > > > >
> > > > > > > From: Tian, Kevin
> > > > > > > Sent: Thursday, July 9, 2020 9:57 AM
> > > > > > >
> > > > > > > > From: Liu, Yi L
> > > > > > > > Sent: Thursday, July 9, 2020 8:32 AM
> > > > > > > >
> > > > > > > > Hi Alex,
> > > > > > > >
> > > > > > > > > Alex Williamson
> > > > > > > > > Sent: Thursday, July 9, 2020 3:55 AM
> > > > > > > > >
> > > > > > > > > On Wed, 8 Jul 2020 08:16:16 + "Liu, Yi L"
> > > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > > Hi Alex,
> > > > > > > > > >
> > > > > > > > > > > From: Liu, Yi L < yi.l@intel.com>
> > > > > > > > > > > Sent: Friday, July 3, 2020 2:28 PM
> > > > > > > > > > >
> > > > > > > > > > > Hi Alex,
> > > > > > > > > > >
> > > > > > > > > > > > From: Alex Williamson
> > > > > > > > > > > > Sent: Friday, July 3, 2020 5:19 AM
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, 24 Jun 2020 01:55:19 -0700 Liu Yi L
> > > > > > > > > > > > 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);
> > > > > > > > > > > > > +
>
RE: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
Hi Alex,
> From: Alex Williamson
> Sent: Thursday, July 9, 2020 10:28 PM
>
> On Thu, 9 Jul 2020 07:16:31 +
> "Liu, Yi L" wrote:
>
> > Hi Alex,
> >
> > After more thinking, looks like adding a r-b tree is still not enough to
> > solve the potential problem for free a range of PASID in one ioctl. If
> > caller gives [0, MAX_UNIT] in the free request, kernel anyhow should
> > loop all the PASIDs and search in the r-b tree. Even VFIO can track the
> > smallest/largest allocated PASID, and limit the free range to an accurate
> > range, it is still no efficient. For example, user has allocated two PASIDs
> > ( 1 and 999), and user gives the [0, MAX_UNIT] range in free request. VFIO
> > will limit the free range to be [1, 999], but still needs to loop PASID 1 -
> > 999, and search in r-b tree.
>
> That sounds like a poor tree implementation. Look at vfio_find_dma()
> for instance, it returns a node within the specified range. If the
> tree has two nodes within the specified range we should never need to
> call a search function like vfio_find_dma() more than three times. We
> call it once, get the first node, remove it. Call it again, get the
> other node, remove it. Call a third time, find no matches, we're done.
> So such an implementation limits searches to N+1 where N is the number
> of nodes within the range.
I see. When getting a free range from user. Use the range to find suited
PASIDs in the r-b tree. For the example I mentioned, if giving [0, MAX_UNIT],
will find two nodes. If giving [0, 100] range, then only one node will be
found. But even though, it still take some time if the user holds a bunch
of PASIDs and user gives a big free range.
> > So I'm wondering can we fall back to prior proposal which only free one
> > PASID for a free request. how about your opinion?
>
> Doesn't it still seem like it would be a useful user interface to have
> a mechanism to free all pasids, by calling with exactly [0, MAX_UINT]?
> I'm not sure if there's another use case for this given than the user
> doesn't have strict control of the pasid values they get. Thanks,
I don't have such use case neither. perhaps we may allow it in future by
adding flag. but if it's still useful, I may try with your suggestion. :-)
Regards,
Yi Liu
> Alex
>
> > > From: Liu, Yi L
> > > Sent: Thursday, July 9, 2020 10:26 AM
> > >
> > > Hi Kevin,
> > >
> > > > From: Tian, Kevin
> > > > Sent: Thursday, July 9, 2020 10:18 AM
> > > >
> > > > > From: Liu, Yi L
> > > > > Sent: Thursday, July 9, 2020 10:08 AM
> > > > >
> > > > > Hi Kevin,
> > > > >
> > > > > > From: Tian, Kevin
> > > > > > Sent: Thursday, July 9, 2020 9:57 AM
> > > > > >
> > > > > > > From: Liu, Yi L
> > > > > > > Sent: Thursday, July 9, 2020 8:32 AM
> > > > > > >
> > > > > > > Hi Alex,
> > > > > > >
> > > > > > > > Alex Williamson
> > > > > > > > Sent: Thursday, July 9, 2020 3:55 AM
> > > > > > > >
> > > > > > > > On Wed, 8 Jul 2020 08:16:16 + "Liu, Yi L"
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Hi Alex,
> > > > > > > > >
> > > > > > > > > > From: Liu, Yi L < yi.l@intel.com>
> > > > > > > > > > Sent: Friday, July 3, 2020 2:28 PM
> > > > > > > > > >
> > > > > > > > > > Hi Alex,
> > > > > > > > > >
> > > > > > > > > > > From: Alex Williamson
> > > > > > > > > > > Sent: Friday, July 3, 2020 5:19 AM
> > > > > > > > > > >
> > > > > > > > > > > On Wed, 24 Jun 2020 01:55:19 -0700 Liu Yi L
> > > > > > > > > > > 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(, (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
Re: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
On Thu, 9 Jul 2020 08:27:51 -0600 Alex Williamson wrote: > > So I'm wondering can we fall back to prior proposal which only free > > one PASID for a free request. how about your opinion? > > Doesn't it still seem like it would be a useful user interface to have > a mechanism to free all pasids, by calling with exactly [0, MAX_UINT]? > I'm not sure if there's another use case for this given than the user > doesn't have strict control of the pasid values they get. Thanks, Yes, I agree free all pasids of a guest is a useful interface. Since all PASIDs under one VM is already tracked by an IOASID set with its XArray, I don't see a need to track again in VFIO. Shall we only free one & free all? IMHO, free range isn't that useful and not really symmetric to PASID allocation in that allocation is one at a time. Can we just add a new flag, e.g. VFIO_IOMMU_FREE_ALL_PASID, and ignored th range in free? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
On Thu, 9 Jul 2020 07:16:31 +
"Liu, Yi L" wrote:
> Hi Alex,
>
> After more thinking, looks like adding a r-b tree is still not enough to
> solve the potential problem for free a range of PASID in one ioctl. If
> caller gives [0, MAX_UNIT] in the free request, kernel anyhow should
> loop all the PASIDs and search in the r-b tree. Even VFIO can track the
> smallest/largest allocated PASID, and limit the free range to an accurate
> range, it is still no efficient. For example, user has allocated two PASIDs
> ( 1 and 999), and user gives the [0, MAX_UNIT] range in free request. VFIO
> will limit the free range to be [1, 999], but still needs to loop PASID 1 -
> 999, and search in r-b tree.
That sounds like a poor tree implementation. Look at vfio_find_dma()
for instance, it returns a node within the specified range. If the
tree has two nodes within the specified range we should never need to
call a search function like vfio_find_dma() more than three times. We
call it once, get the first node, remove it. Call it again, get the
other node, remove it. Call a third time, find no matches, we're done.
So such an implementation limits searches to N+1 where N is the number
of nodes within the range.
> So I'm wondering can we fall back to prior proposal which only free one
> PASID for a free request. how about your opinion?
Doesn't it still seem like it would be a useful user interface to have
a mechanism to free all pasids, by calling with exactly [0, MAX_UINT]?
I'm not sure if there's another use case for this given than the user
doesn't have strict control of the pasid values they get. Thanks,
Alex
> > From: Liu, Yi L
> > Sent: Thursday, July 9, 2020 10:26 AM
> >
> > Hi Kevin,
> >
> > > From: Tian, Kevin
> > > Sent: Thursday, July 9, 2020 10:18 AM
> > >
> > > > From: Liu, Yi L
> > > > Sent: Thursday, July 9, 2020 10:08 AM
> > > >
> > > > Hi Kevin,
> > > >
> > > > > From: Tian, Kevin
> > > > > Sent: Thursday, July 9, 2020 9:57 AM
> > > > >
> > > > > > From: Liu, Yi L
> > > > > > Sent: Thursday, July 9, 2020 8:32 AM
> > > > > >
> > > > > > Hi Alex,
> > > > > >
> > > > > > > Alex Williamson
> > > > > > > Sent: Thursday, July 9, 2020 3:55 AM
> > > > > > >
> > > > > > > On Wed, 8 Jul 2020 08:16:16 + "Liu, Yi L"
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Hi Alex,
> > > > > > > >
> > > > > > > > > From: Liu, Yi L < yi.l@intel.com>
> > > > > > > > > Sent: Friday, July 3, 2020 2:28 PM
> > > > > > > > >
> > > > > > > > > Hi Alex,
> > > > > > > > >
> > > > > > > > > > From: Alex Williamson
> > > > > > > > > > Sent: Friday, July 3, 2020 5:19 AM
> > > > > > > > > >
> > > > > > > > > > On Wed, 24 Jun 2020 01:55:19 -0700 Liu Yi L
> > > > > > > > > > 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(, (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
RE: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
Hi Alex,
After more thinking, looks like adding a r-b tree is still not enough to
solve the potential problem for free a range of PASID in one ioctl. If
caller gives [0, MAX_UNIT] in the free request, kernel anyhow should
loop all the PASIDs and search in the r-b tree. Even VFIO can track the
smallest/largest allocated PASID, and limit the free range to an accurate
range, it is still no efficient. For example, user has allocated two PASIDs
( 1 and 999), and user gives the [0, MAX_UNIT] range in free request. VFIO
will limit the free range to be [1, 999], but still needs to loop PASID 1 -
999, and search in r-b tree.
So I'm wondering can we fall back to prior proposal which only free one
PASID for a free request. how about your opinion?
https://lore.kernel.org/linux-iommu/20200416084031.7266a...@w520.home/
Regards,
Yi Liu
> From: Liu, Yi L
> Sent: Thursday, July 9, 2020 10:26 AM
>
> Hi Kevin,
>
> > From: Tian, Kevin
> > Sent: Thursday, July 9, 2020 10:18 AM
> >
> > > From: Liu, Yi L
> > > Sent: Thursday, July 9, 2020 10:08 AM
> > >
> > > Hi Kevin,
> > >
> > > > From: Tian, Kevin
> > > > Sent: Thursday, July 9, 2020 9:57 AM
> > > >
> > > > > From: Liu, Yi L
> > > > > Sent: Thursday, July 9, 2020 8:32 AM
> > > > >
> > > > > Hi Alex,
> > > > >
> > > > > > Alex Williamson
> > > > > > Sent: Thursday, July 9, 2020 3:55 AM
> > > > > >
> > > > > > On Wed, 8 Jul 2020 08:16:16 + "Liu, Yi L"
> > > > > > wrote:
> > > > > >
> > > > > > > Hi Alex,
> > > > > > >
> > > > > > > > From: Liu, Yi L < yi.l@intel.com>
> > > > > > > > Sent: Friday, July 3, 2020 2:28 PM
> > > > > > > >
> > > > > > > > Hi Alex,
> > > > > > > >
> > > > > > > > > From: Alex Williamson
> > > > > > > > > Sent: Friday, July 3, 2020 5:19 AM
> > > > > > > > >
> > > > > > > > > On Wed, 24 Jun 2020 01:55:19 -0700 Liu Yi L
> > > > > > > > > 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(, (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
> > > >
> > > > Agree. I thought ioasid_free should at least carry a token since
> > > > the user
> > > space is
> > > > only allowed to manage PASIDs in its own
RE: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
Hi Kevin,
> From: Tian, Kevin
> Sent: Thursday, July 9, 2020 10:18 AM
>
> > From: Liu, Yi L
> > Sent: Thursday, July 9, 2020 10:08 AM
> >
> > Hi Kevin,
> >
> > > From: Tian, Kevin
> > > Sent: Thursday, July 9, 2020 9:57 AM
> > >
> > > > From: Liu, Yi L
> > > > Sent: Thursday, July 9, 2020 8:32 AM
> > > >
> > > > Hi Alex,
> > > >
> > > > > Alex Williamson
> > > > > Sent: Thursday, July 9, 2020 3:55 AM
> > > > >
> > > > > On Wed, 8 Jul 2020 08:16:16 +
> > > > > "Liu, Yi L" wrote:
> > > > >
> > > > > > Hi Alex,
> > > > > >
> > > > > > > From: Liu, Yi L < yi.l@intel.com>
> > > > > > > Sent: Friday, July 3, 2020 2:28 PM
> > > > > > >
> > > > > > > Hi Alex,
> > > > > > >
> > > > > > > > From: Alex Williamson
> > > > > > > > Sent: Friday, July 3, 2020 5:19 AM
> > > > > > > >
> > > > > > > > On Wed, 24 Jun 2020 01:55:19 -0700 Liu Yi L
> > > > > > > > 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(, (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
> > >
> > > Agree. I thought ioasid_free should at least carry a token since the user
> > space is
> > > only allowed to manage PASIDs in its own set...
> > >
> > > > > 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.
> > > >
> > >
> > > check current ioasid_free:
> > >
> > > spin_lock(_allocator_lock);
> > > ioasid_data = xa_load(_allocator->xa, ioasid);
> > > if (!ioasid_data) {
> > > pr_err("Trying to free unknown IOASID %u\n", ioasid);
> > > goto exit_unlock;
> > > }
> > >
> > > Allow an user to trigger above lock paths with MAX_UINT times might still
> > be bad.
> >
> > yeah, how about the below two options:
> >
> > - comparing the max - min with the quota before calling ioasid_free().
> > If max - min > current quota of the user, then should fail it. If
> > max - min < quota, then call ioasid_free() one by one. still trigger
> > the above lock path
RE: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
> From: Liu, Yi L
> Sent: Thursday, July 9, 2020 10:08 AM
>
> Hi Kevin,
>
> > From: Tian, Kevin
> > Sent: Thursday, July 9, 2020 9:57 AM
> >
> > > From: Liu, Yi L
> > > Sent: Thursday, July 9, 2020 8:32 AM
> > >
> > > Hi Alex,
> > >
> > > > Alex Williamson
> > > > Sent: Thursday, July 9, 2020 3:55 AM
> > > >
> > > > On Wed, 8 Jul 2020 08:16:16 +
> > > > "Liu, Yi L" wrote:
> > > >
> > > > > Hi Alex,
> > > > >
> > > > > > From: Liu, Yi L < yi.l@intel.com>
> > > > > > Sent: Friday, July 3, 2020 2:28 PM
> > > > > >
> > > > > > Hi Alex,
> > > > > >
> > > > > > > From: Alex Williamson
> > > > > > > Sent: Friday, July 3, 2020 5:19 AM
> > > > > > >
> > > > > > > On Wed, 24 Jun 2020 01:55:19 -0700 Liu Yi L
> > > > > > > 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(, (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
> >
> > Agree. I thought ioasid_free should at least carry a token since the user
> space is
> > only allowed to manage PASIDs in its own set...
> >
> > > > 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.
> > >
> >
> > check current ioasid_free:
> >
> > spin_lock(_allocator_lock);
> > ioasid_data = xa_load(_allocator->xa, ioasid);
> > if (!ioasid_data) {
> > pr_err("Trying to free unknown IOASID %u\n", ioasid);
> > goto exit_unlock;
> > }
> >
> > Allow an user to trigger above lock paths with MAX_UINT times might still
> be bad.
>
> yeah, how about the below two options:
>
> - comparing the max - min with the quota before calling ioasid_free().
> If max - min > current quota of the user, then should fail it. If
> max - min < quota, then call ioasid_free() one by one. still trigger
> the above lock path with quota times.
This is definitely wrong. [min, max] is about the range of the PASID value,
while quota is about the number of allocated PASIDs. It's a bit weird to
mix two together. btw what is the main purpose of allowing batch PASID
free requests? Can we just simplify to allow one PASID in each
RE: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
Hi Kevin,
> From: Tian, Kevin
> Sent: Thursday, July 9, 2020 9:57 AM
>
> > From: Liu, Yi L
> > Sent: Thursday, July 9, 2020 8:32 AM
> >
> > Hi Alex,
> >
> > > Alex Williamson
> > > Sent: Thursday, July 9, 2020 3:55 AM
> > >
> > > On Wed, 8 Jul 2020 08:16:16 +
> > > "Liu, Yi L" wrote:
> > >
> > > > Hi Alex,
> > > >
> > > > > From: Liu, Yi L < yi.l@intel.com>
> > > > > Sent: Friday, July 3, 2020 2:28 PM
> > > > >
> > > > > Hi Alex,
> > > > >
> > > > > > From: Alex Williamson
> > > > > > Sent: Friday, July 3, 2020 5:19 AM
> > > > > >
> > > > > > On Wed, 24 Jun 2020 01:55:19 -0700 Liu Yi L
> > > > > > 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(, (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
>
> Agree. I thought ioasid_free should at least carry a token since the user
> space is
> only allowed to manage PASIDs in its own set...
>
> > > 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.
> >
>
> check current ioasid_free:
>
> spin_lock(_allocator_lock);
> ioasid_data = xa_load(_allocator->xa, ioasid);
> if (!ioasid_data) {
> pr_err("Trying to free unknown IOASID %u\n", ioasid);
> goto exit_unlock;
> }
>
> Allow an user to trigger above lock paths with MAX_UINT times might still be
> bad.
yeah, how about the below two options:
- comparing the max - min with the quota before calling ioasid_free().
If max - min > current quota of the user, then should fail it. If
max - min < quota, then call ioasid_free() one by one. still trigger
the above lock path with quota times.
- pass the max and min to ioasid_free(), let ioasid_free() decide. should
be able to avoid trigger the lock multiple times, and ioasid has have a
track on how may PASIDs have been allocated, if max - min is larger than
the allocated number, should fail anyway.
thoughts on the above reply?
Regards,
Yi Liu
> Thanks
> Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
> From: Liu, Yi L
> Sent: Thursday, July 9, 2020 8:32 AM
>
> Hi Alex,
>
> > Alex Williamson
> > Sent: Thursday, July 9, 2020 3:55 AM
> >
> > On Wed, 8 Jul 2020 08:16:16 +
> > "Liu, Yi L" wrote:
> >
> > > Hi Alex,
> > >
> > > > From: Liu, Yi L < yi.l@intel.com>
> > > > Sent: Friday, July 3, 2020 2:28 PM
> > > >
> > > > Hi Alex,
> > > >
> > > > > From: Alex Williamson
> > > > > Sent: Friday, July 3, 2020 5:19 AM
> > > > >
> > > > > On Wed, 24 Jun 2020 01:55:19 -0700
> > > > > Liu Yi L 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(, (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
Agree. I thought ioasid_free should at least carry a token since the
user space is only allowed to manage PASIDs in its own set...
> > 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.
>
check current ioasid_free:
spin_lock(_allocator_lock);
ioasid_data = xa_load(_allocator->xa, ioasid);
if (!ioasid_data) {
pr_err("Trying to free unknown IOASID %u\n", ioasid);
goto exit_unlock;
}
Allow an user to trigger above lock paths with MAX_UINT times might still
be bad.
Thanks
Kevin
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
Hi Alex,
> Alex Williamson
> Sent: Thursday, July 9, 2020 3:55 AM
>
> On Wed, 8 Jul 2020 08:16:16 +
> "Liu, Yi L" wrote:
>
> > Hi Alex,
> >
> > > From: Liu, Yi L < yi.l@intel.com>
> > > Sent: Friday, July 3, 2020 2:28 PM
> > >
> > > Hi Alex,
> > >
> > > > From: Alex Williamson
> > > > Sent: Friday, July 3, 2020 5:19 AM
> > > >
> > > > On Wed, 24 Jun 2020 01:55:19 -0700
> > > > Liu Yi L 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(, (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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
On Wed, 8 Jul 2020 08:16:16 +
"Liu, Yi L" wrote:
> Hi Alex,
>
> > From: Liu, Yi L < yi.l@intel.com>
> > Sent: Friday, July 3, 2020 2:28 PM
> >
> > Hi Alex,
> >
> > > From: Alex Williamson
> > > Sent: Friday, July 3, 2020 5:19 AM
> > >
> > > On Wed, 24 Jun 2020 01:55:19 -0700
> > > Liu Yi L 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(, (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.
Thanks,
Alex
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
Hi Alex,
> From: Liu, Yi L < yi.l@intel.com>
> Sent: Friday, July 3, 2020 2:28 PM
>
> Hi Alex,
>
> > From: Alex Williamson
> > Sent: Friday, July 3, 2020 5:19 AM
> >
> > On Wed, 24 Jun 2020 01:55:19 -0700
> > Liu Yi L 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(, (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?
Regards,
Yi Liu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
Hi Alex,
> From: Alex Williamson
> Sent: Friday, July 3, 2020 5:19 AM
>
> On Wed, 24 Jun 2020 01:55:19 -0700
> Liu Yi L 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.
> >
> > Cc: Kevin Tian
> > CC: Jacob Pan
> > Cc: Alex Williamson
> > Cc: Eric Auger
> > Cc: Jean-Philippe Brucker
> > Cc: Joerg Roedel
> > Cc: Lu Baolu
> > Signed-off-by: Liu Yi L
> > Signed-off-by: Yi Sun
> > Signed-off-by: Jacob Pan
> > ---
> > v1 -> v2:
> > *) move the vfio_mm related code to be a seprate module
> > *) use a single structure for alloc/free, could support a range of
> > PASIDs
> > *) fetch vfio_mm at group_attach time instead of at iommu driver open
> > time
> > ---
> > drivers/vfio/Kconfig| 1 +
> > drivers/vfio/vfio_iommu_type1.c | 96
> -
> > drivers/vfio/vfio_pasid.c | 10 +
> > include/linux/vfio.h| 6 +++
> > include/uapi/linux/vfio.h | 36
> > 5 files changed, 147 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig index
> > 3d8a108..95d90c6 100644
> > --- a/drivers/vfio/Kconfig
> > +++ b/drivers/vfio/Kconfig
> > @@ -2,6 +2,7 @@
> > config VFIO_IOMMU_TYPE1
> > tristate
> > depends on VFIO
> > + select VFIO_PASID if (X86)
> > default n
> >
> > config VFIO_IOMMU_SPAPR_TCE
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c index 8c143d5..d0891c5 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -73,6 +73,7 @@ struct vfio_iommu {
> > boolv2;
> > boolnesting;
> > struct iommu_nesting_info *nesting_info;
> > + struct vfio_mm *vmm;
>
> Structure alignment again.
sure. may get agreement in the prior email.
>
> > booldirty_page_tracking;
> > boolpinned_page_dirty_scope;
> > };
> > @@ -1933,6 +1934,17 @@ static void vfio_iommu_iova_insert_copy(struct
> > vfio_iommu *iommu,
> >
> > list_splice_tail(iova_copy, iova);
> > }
> > +
> > +static void vfio_iommu_release_nesting_info(struct vfio_iommu *iommu)
> > +{
> > + if (iommu->vmm) {
> > + vfio_mm_put(iommu->vmm);
> > + iommu->vmm = NULL;
> > + }
> > +
> > + kfree(iommu->nesting_info);
>
> iommu->nesting_info = NULL;
got it.
> > +}
> > +
> > static int vfio_iommu_type1_attach_group(void *iommu_data,
> > struct iommu_group *iommu_group)
> { @@ -2067,6 +2079,25 @@
> > static int vfio_iommu_type1_attach_group(void *iommu_data,
> > goto out_detach;
> > }
> > iommu->nesting_info = info;
> > +
> > + if (info->features & IOMMU_NESTING_FEAT_SYSWIDE_PASID) {
> > + struct vfio_mm *vmm;
> > + int sid;
> > +
> > + vmm = vfio_mm_get_from_task(current);
> > + if (IS_ERR(vmm)) {
> > + ret = PTR_ERR(vmm);
> > + goto out_detach;
> > + }
> > + iommu->vmm = vmm;
> > +
> > + sid = vfio_mm_ioasid_sid(vmm);
> > + ret = iommu_domain_set_attr(domain->domain,
> > + DOMAIN_ATTR_IOASID_SID,
> > + );
>
> This looks pretty dicey in the case of !CONFIG_VFIO_PASID, can we get here in
> that case? If so it looks like we're doing bad things with setting the
> domain-
> >ioasid_sid.
I guess not. So far, vfio_iommu_type1 will select CONFIG_VFIO_PASID for X86.
do you think it is enough?
>
> > + if (ret)
> > + goto out_detach;
> > + }
> > }
> >
> > /* Get aperture info */
> > @@ -2178,7 +2209,8 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> > return 0;
> >
> > out_detach:
> > - kfree(iommu->nesting_info);
> > + if (iommu->nesting_info)
> > + vfio_iommu_release_nesting_info(iommu);
>
> Make vfio_iommu_release_nesting_info() check iommu->nesting_info, then call
> it unconditionally?
got it. :-)
> > vfio_iommu_detach_group(domain, group);
> > out_domain:
> > iommu_domain_free(domain->domain);
> > @@ -2380,7 +2412,8 @@ static void vfio_iommu_type1_detach_group(void
> *iommu_data,
> > else
> >
> vfio_iommu_unmap_unpin_reaccount(iommu);
> >
> > - kfree(iommu->nesting_info);
> > + if (iommu->nesting_info)
> > +
> vfio_iommu_release_nesting_info(iommu);
> > }
> > iommu_domain_free(domain->domain);
> >
Re: [PATCH v3 06/14] vfio/type1: Add VFIO_IOMMU_PASID_REQUEST (alloc/free)
On Wed, 24 Jun 2020 01:55:19 -0700
Liu Yi L 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.
>
> Cc: Kevin Tian
> CC: Jacob Pan
> Cc: Alex Williamson
> Cc: Eric Auger
> Cc: Jean-Philippe Brucker
> Cc: Joerg Roedel
> Cc: Lu Baolu
> Signed-off-by: Liu Yi L
> Signed-off-by: Yi Sun
> Signed-off-by: Jacob Pan
> ---
> v1 -> v2:
> *) move the vfio_mm related code to be a seprate module
> *) use a single structure for alloc/free, could support a range of PASIDs
> *) fetch vfio_mm at group_attach time instead of at iommu driver open time
> ---
> drivers/vfio/Kconfig| 1 +
> drivers/vfio/vfio_iommu_type1.c | 96
> -
> drivers/vfio/vfio_pasid.c | 10 +
> include/linux/vfio.h| 6 +++
> include/uapi/linux/vfio.h | 36
> 5 files changed, 147 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> index 3d8a108..95d90c6 100644
> --- a/drivers/vfio/Kconfig
> +++ b/drivers/vfio/Kconfig
> @@ -2,6 +2,7 @@
> config VFIO_IOMMU_TYPE1
> tristate
> depends on VFIO
> + select VFIO_PASID if (X86)
> default n
>
> config VFIO_IOMMU_SPAPR_TCE
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 8c143d5..d0891c5 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -73,6 +73,7 @@ struct vfio_iommu {
> boolv2;
> boolnesting;
> struct iommu_nesting_info *nesting_info;
> + struct vfio_mm *vmm;
Structure alignment again.
> booldirty_page_tracking;
> boolpinned_page_dirty_scope;
> };
> @@ -1933,6 +1934,17 @@ static void vfio_iommu_iova_insert_copy(struct
> vfio_iommu *iommu,
>
> list_splice_tail(iova_copy, iova);
> }
> +
> +static void vfio_iommu_release_nesting_info(struct vfio_iommu *iommu)
> +{
> + if (iommu->vmm) {
> + vfio_mm_put(iommu->vmm);
> + iommu->vmm = NULL;
> + }
> +
> + kfree(iommu->nesting_info);
iommu->nesting_info = NULL;
> +}
> +
> static int vfio_iommu_type1_attach_group(void *iommu_data,
>struct iommu_group *iommu_group)
> {
> @@ -2067,6 +2079,25 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> goto out_detach;
> }
> iommu->nesting_info = info;
> +
> + if (info->features & IOMMU_NESTING_FEAT_SYSWIDE_PASID) {
> + struct vfio_mm *vmm;
> + int sid;
> +
> + vmm = vfio_mm_get_from_task(current);
> + if (IS_ERR(vmm)) {
> + ret = PTR_ERR(vmm);
> + goto out_detach;
> + }
> + iommu->vmm = vmm;
> +
> + sid = vfio_mm_ioasid_sid(vmm);
> + ret = iommu_domain_set_attr(domain->domain,
> + DOMAIN_ATTR_IOASID_SID,
> + );
This looks pretty dicey in the case of !CONFIG_VFIO_PASID, can we get
here in that case? If so it looks like we're doing bad things with
setting the domain->ioasid_sid.
> + if (ret)
> + goto out_detach;
> + }
> }
>
> /* Get aperture info */
> @@ -2178,7 +2209,8 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> return 0;
>
> out_detach:
> - kfree(iommu->nesting_info);
> + if (iommu->nesting_info)
> + vfio_iommu_release_nesting_info(iommu);
Make vfio_iommu_release_nesting_info() check iommu->nesting_info, then
call it unconditionally?
> vfio_iommu_detach_group(domain, group);
> out_domain:
> iommu_domain_free(domain->domain);
> @@ -2380,7 +2412,8 @@ static void vfio_iommu_type1_detach_group(void
> *iommu_data,
> else
> vfio_iommu_unmap_unpin_reaccount(iommu);
>
> - kfree(iommu->nesting_info);
> + if (iommu->nesting_info)
> + vfio_iommu_release_nesting_info(iommu);
> }
> iommu_domain_free(domain->domain);
> list_del(>next);
> @@ -2852,6 +2885,63 @@ static int vfio_iommu_type1_dirty_pages(struct
> vfio_iommu *iommu,
> return -EINVAL;
> }
>
> +static int vfio_iommu_type1_pasid_alloc(struct vfio_iommu *iommu,
> + unsigned int min,
> + unsigned int max)
