On 4/29/24 10:39 AM, Tian, Kevin wrote:
From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
Sent: Sunday, April 28, 2024 6:22 PM
On 2024/4/10 7:48, Jason Gunthorpe wrote:
On Tue, Apr 09, 2024 at 10:11:28AM +0800, Baolu Lu wrote:
On 4/8/24 10:19 PM, Jason Gunthorpe wrote:
On Sat, Apr 06, 2024 at 02:09:34PM +0800, Baolu Lu wrote:
On 4/3/24 7:59 PM, Jason Gunthorpe wrote:
On Wed, Apr 03, 2024 at 09:15:12AM +0800, Lu Baolu wrote:
+ /* A bond already exists, just take a reference`. */
+ handle = iommu_attach_handle_get(group, iommu_mm-
pasid);
+ if (handle) {
+ mutex_unlock(&iommu_sva_lock);
+ return handle;
}
At least in this context this is not enough we need to ensure that the
domain on the PASID is actually an SVA domain and it was installed by
this mechanism, not an iommufd domain for instance.
ie you probably need a type field in the iommu_attach_handle to tell
what the priv is.
Otherwise this seems like a great idea!
Yes, you are right. For the SVA case, I will add the following changes.
The IOMMUFD path will also need such enhancement. I will update it in
the next version.
The only use for this is the PRI callbacks right? Maybe instead of
adding a handle type let's just check domain->iopf_handler ?
Ie SVA will pass &ommu_sva_iopf_handler as its "type"
Sorry that I don't fully understand the proposal here.
I was talking specifically about the type field you suggested adding
to the handle struct.
Instead of adding a type field check the domain->iopf_handler to
determine the domain and thus handle type.
The problem is that the context code (SVA, IOMMUFD, etc.) needs to
make
sure that the attach handle is really what it has installed during
domain attachment. The context code needs some mechanism to include
some
kind of "owner cookie" in the attach handle, so that it could check
against it later for valid use.
Right, you have a derived struct for each user and you need a way to
check if casting from the general handle struct to the derived struct
is OK.
I'm suggesting using domain->iopf_handle as the type key.
After removing the refcount from the attach handle, I am trying to make
the code look like this,
/* A bond already exists, just take a reference`. */
handle = iommu_attach_handle_get(group, iommu_mm->pasid);
if (handle) {
if (handle->domain->iopf_handler !=
iommu_sva_iopf_handler) {
ret = -EBUSY;
goto out_unlock;
}
refcount_inc(&handle->users);
mutex_unlock(&iommu_sva_lock);
return handle;
}
But it appears that this code is not lock safe. If the domain on the
PASID is not a SVA domain, the check of "handle->domain->iopf_handler !=
iommu_sva_iopf_handler" could result in a use-after-free issue as the
other thread might detach the domain in between the fetch and check
lines.
Probably we still need to keep the refcount in the attach handle?
What about Jason's another comment in his original replies?
"
Though I'm not convinced the refcount should be elevated into the core
structure. The prior patch I showed you where the caller can provide
the memory for the handle and we don't have a priv would make it easy
to put the refcount in a SVA dervied handle struct without more
allocation. Then we don't need this weirdness.
"
That sounds like we'll need a iommu_sva like structure to hold
its own refcnt. Then we don't need this type check and refcnt
in the core.
The problem I'm facing isn't about who allocates the handle memory.
Instead, there's no mechanism to synchronize access between two threads.
One thread might remove the handle while another fetches and reads a
member of its structure.
A similar issue exists with iommu_get_domain_for_dev_pasid(). It fetches
and returns a domain, but there's no guarantee that the domain will
*not* be freed while the caller is still using it.
One reason I introduced the reference count for attach handles is to
potentially replace iommu_get_domain_for_dev_pasid(), allowing the
domain to be accessible without any potential UAF issue.
Best regards,
baolu
