There is a slight difference between iopf domains and non-iopf domains.
In the latter, references to domains occur between attach and detach;
While in the former, due to the existence of asynchronous iopf handling
paths, references to the domain may occur after detach, which leads to
potential UAF issues.
Introduce iopf-specific domain attach/detach/replace interface where the
caller provides an attach cookie. This cookie can only be freed after all
outstanding iopf groups are handled and the domain is detached from the
RID or PASID. The presence of this attach cookie indicates that a domain
has been attached to the RID or PASID and won't be released until all
outstanding iopf groups are handled.
The cookie data structure also includes a private field for storing a
caller-specific pointer that will be passed back to its page fault handler.
This field provides flexibility for various uses. For example, the IOMMUFD
could use it to store the iommufd_device pointer, so that it could easily
retrieve the dev_id of the device that triggered the fault.
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 36 +++++++++
drivers/iommu/io-pgfault.c | 158 +++++++++++++++++++++++++++++++++++++
2 files changed, 194 insertions(+)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 1ccad10e8164..6d85be23952a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -120,6 +120,16 @@ struct iommu_page_response {
u32 code;
};
+struct iopf_attach_cookie {
+ struct iommu_domain *domain;
+ struct device *dev;
+ unsigned int pasid;
+ refcount_t users;
+
+ void *private;
+ void (*release)(struct iopf_attach_cookie *cookie);
+};
+
struct iopf_fault {
struct iommu_fault fault;
/* node for pending lists */
@@ -699,6 +709,7 @@ struct iommu_fault_param {
struct device *dev;
struct iopf_queue *queue;
struct list_head queue_list;
+ struct xarray pasid_cookie;
struct list_head partial;
struct list_head faults;
@@ -1552,6 +1563,12 @@ void iopf_free_group(struct iopf_group *group);
void iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
void iopf_group_response(struct iopf_group *group,
enum iommu_page_response_code status);
+int iopf_domain_attach(struct iommu_domain *domain, struct device *dev,
+ ioasid_t pasid, struct iopf_attach_cookie *cookie);
+void iopf_domain_detach(struct iommu_domain *domain, struct device *dev,
+ ioasid_t pasid);
+int iopf_domain_replace(struct iommu_domain *domain, struct device *dev,
+ ioasid_t pasid, struct iopf_attach_cookie *cookie);
#else
static inline int
iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
@@ -1596,5 +1613,24 @@ static inline void iopf_group_response(struct iopf_group *group,
enum iommu_page_response_code status)
{
}
+
+static inline int iopf_domain_attach(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid,
+ struct iopf_attach_cookie *cookie)
+{
+ return -ENODEV;
+}
+
+static inline void iopf_domain_detach(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid)
+{
+}
+
+static inline int iopf_domain_replace(struct iommu_domain *domain,
+ struct device *dev, ioasid_t pasid,
+ struct iopf_attach_cookie *cookie)
+{
+ return -ENODEV;
+}
#endif /* CONFIG_IOMMU_IOPF */
#endif /* __LINUX_IOMMU_H */
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index b64229dab976..f7ce41573799 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -39,6 +39,103 @@ static void iopf_put_dev_fault_param(struct iommu_fault_param *fault_param)
kfree_rcu(fault_param, rcu);
}
+/* Get the domain attachment cookie for pasid of a device. */
+static struct iopf_attach_cookie __maybe_unused *
+iopf_pasid_cookie_get(struct device *dev, ioasid_t pasid)
+{
+ struct iommu_fault_param *iopf_param = iopf_get_dev_fault_param(dev);
+ struct iopf_attach_cookie *curr;
+
+ if (!iopf_param)
+ return ERR_PTR(-ENODEV);
+
+ xa_lock(&iopf_param->pasid_cookie);
+ curr = xa_load(&iopf_param->pasid_cookie, pasid);
+ if (curr && !refcount_inc_not_zero(&curr->users))
+ curr = ERR_PTR(-EINVAL);
+ xa_unlock(&iopf_param->pasid_cookie);
+
+ iopf_put_dev_fault_param(iopf_param);
+
+ return curr;
+}
+
+/* Put the domain attachment cookie. */
+static void iopf_pasid_cookie_put(struct iopf_attach_cookie *cookie)
+{
+ if (cookie && refcount_dec_and_test(&cookie->users))
+ cookie->release(cookie);
+}
+
+/*
+ * Set the domain attachment cookie for pasid of a device. Return 0 on
+ * success, or error number on failure.
+ */
+static int iopf_pasid_cookie_set(struct iommu_domain *domain, struct device *dev,
+ ioasid_t pasid, struct iopf_attach_cookie *cookie)
+{
+ struct iommu_fault_param *iopf_param = iopf_get_dev_fault_param(dev);
+ struct iopf_attach_cookie *curr;
+
+ if (!iopf_param)
+ return -ENODEV;
+
+ refcount_set(&cookie->users, 1);
+ cookie->dev = dev;
+ cookie->pasid = pasid;
+ cookie->domain = domain;
+
+ curr = xa_cmpxchg(&iopf_param->pasid_cookie, pasid, NULL, cookie, GFP_KERNEL);
+ iopf_put_dev_fault_param(iopf_param);
+
+ return curr ? xa_err(curr) : 0;
+}
+
+/* Clear the domain attachment cookie for pasid of a device. */
+static void iopf_pasid_cookie_clear(struct device *dev, ioasid_t pasid)
+{
+ struct iommu_fault_param *iopf_param = iopf_get_dev_fault_param(dev);
+ struct iopf_attach_cookie *curr;
+
+ if (WARN_ON(!iopf_param))
+ return;
+
+ curr = xa_erase(&iopf_param->pasid_cookie, pasid);
+ /* paired with iopf_pasid_cookie_set/replace() */
+ iopf_pasid_cookie_put(curr);
+
+ iopf_put_dev_fault_param(iopf_param);
+}
+
+/* Replace the domain attachment cookie for pasid of a device. */
+static int iopf_pasid_cookie_replace(struct iommu_domain *domain, struct device *dev,
+ ioasid_t pasid, struct iopf_attach_cookie *cookie)
+{
+ struct iommu_fault_param *iopf_param = iopf_get_dev_fault_param(dev);
+ struct iopf_attach_cookie *curr;
+
+ if (!iopf_param)
+ return -ENODEV;
+
+ if (cookie) {
+ refcount_set(&cookie->users, 1);
+ cookie->dev = dev;
+ cookie->pasid = pasid;
+ cookie->domain = domain;
+ }
+
+ curr = xa_store(&iopf_param->pasid_cookie, pasid, cookie, GFP_KERNEL);
+ if (xa_err(curr))
+ return xa_err(curr);
+
+ /* paired with iopf_pasid_cookie_set/replace() */
+ iopf_pasid_cookie_put(curr);
+
+ iopf_put_dev_fault_param(iopf_param);
+
+ return 0;
+}
+
static void __iopf_free_group(struct iopf_group *group)
{
struct iopf_fault *iopf, *next;
@@ -362,6 +459,7 @@ int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
mutex_init(&fault_param->lock);
INIT_LIST_HEAD(&fault_param->faults);
INIT_LIST_HEAD(&fault_param->partial);
+ xa_init(&fault_param->pasid_cookie);
fault_param->dev = dev;
refcount_set(&fault_param->users, 1);
list_add(&fault_param->queue_list, &queue->devices);
@@ -502,3 +600,63 @@ void iopf_queue_free(struct iopf_queue *queue)
kfree(queue);
}
EXPORT_SYMBOL_GPL(iopf_queue_free);
+
+int iopf_domain_attach(struct iommu_domain *domain, struct device *dev,
+ ioasid_t pasid, struct iopf_attach_cookie *cookie)
+{
+ int ret;
+
+ if (!domain->iopf_handler)
+ return -EINVAL;
+
+ if (pasid == IOMMU_NO_PASID)
+ ret = iommu_attach_group(domain, dev->iommu_group);
+ else
+ ret = iommu_attach_device_pasid(domain, dev, pasid);
+ if (ret)
+ return ret;
+
+ ret = iopf_pasid_cookie_set(domain, dev, pasid, cookie);
+ if (ret) {
+ if (pasid == IOMMU_NO_PASID)
+ iommu_detach_group(domain, dev->iommu_group);
+ else
+ iommu_detach_device_pasid(domain, dev, pasid);
+ }
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(iopf_domain_attach);
+
+void iopf_domain_detach(struct iommu_domain *domain, struct device *dev, ioasid_t pasid)
+{
+ iopf_pasid_cookie_clear(dev, pasid);
+
+ if (pasid == IOMMU_NO_PASID)
+ iommu_detach_group(domain, dev->iommu_group);
+ else
+ iommu_detach_device_pasid(domain, dev, pasid);
+}
+EXPORT_SYMBOL_GPL(iopf_domain_detach);
+
+int iopf_domain_replace(struct iommu_domain *domain, struct device *dev,
+ ioasid_t pasid, struct iopf_attach_cookie *cookie)
+{
+ struct iommu_domain *old_domain = iommu_get_domain_for_dev(dev);
+ int ret;
+
+ if (!old_domain || pasid != IOMMU_NO_PASID ||
+ (!old_domain->iopf_handler && !domain->iopf_handler))
+ return -EINVAL;
+
+ ret = iommu_group_replace_domain(dev->iommu_group, domain);
+ if (ret)
+ return ret;
+
+ ret = iopf_pasid_cookie_replace(domain, dev, pasid, cookie);
+ if (ret)
+ iommu_group_replace_domain(dev->iommu_group, old_domain);
+
+ return ret;
+}
+EXPORT_SYMBOL_NS_GPL(iopf_domain_replace, IOMMUFD_INTERNAL);
--
2.34.1
> From: Lu Baolu <[email protected]>
> Sent: Monday, January 22, 2024 3:39 PM
>
> There is a slight difference between iopf domains and non-iopf domains.
> In the latter, references to domains occur between attach and detach;
> While in the former, due to the existence of asynchronous iopf handling
> paths, references to the domain may occur after detach, which leads to
> potential UAF issues.
Does UAF still exist if iommu driver follows the guidance you just added
to iopf_queue_remove_device()?
it clearly says that the driver needs to disable IOMMU PRI reception,
remove device from iopf queue and disable PRI on the device.
presumably those are all about what needs to be done in the detach
operation. Then once detach completes there should be no more
reference to the domain from the iopf path?
>
> +struct iopf_attach_cookie {
> + struct iommu_domain *domain;
> + struct device *dev;
> + unsigned int pasid;
> + refcount_t users;
> +
> + void *private;
> + void (*release)(struct iopf_attach_cookie *cookie);
> +};
this cookie has nothing specific to iopf.
it may makes more sense to build a generic iommu_attach_device_cookie()
helper so the same object can be reused in future other usages too.
within iommu core it can check domain iopf handler and this generic cookie
to update iopf specific data e.g. the pasid_cookie xarray.
On 2024/2/7 16:11, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Monday, January 22, 2024 3:39 PM
>>
>> There is a slight difference between iopf domains and non-iopf domains.
>> In the latter, references to domains occur between attach and detach;
>> While in the former, due to the existence of asynchronous iopf handling
>> paths, references to the domain may occur after detach, which leads to
>> potential UAF issues.
>
> Does UAF still exist if iommu driver follows the guidance you just added
> to iopf_queue_remove_device()?
>
> it clearly says that the driver needs to disable IOMMU PRI reception,
> remove device from iopf queue and disable PRI on the device.
The iopf_queue_remove_device() function is only called after the last
iopf-capable domain is detached from the device. It may not be called
during domain replacement. Hence, there is no guarantee that
iopf_queue_remove_device() will be called when a domain is detached from
the device.
>
> presumably those are all about what needs to be done in the detach
> operation. Then once detach completes there should be no more
> reference to the domain from the iopf path?
The domain pointer stored in the iopf_group structure is only released
after the iopf response, possibly after the domain is detached from the
device. Thus, the domain pointer can only be freed after the iopf
response.
>
>>
>> +struct iopf_attach_cookie {
>> + struct iommu_domain *domain;
>> + struct device *dev;
>> + unsigned int pasid;
>> + refcount_t users;
>> +
>> + void *private;
>> + void (*release)(struct iopf_attach_cookie *cookie);
>> +};
>
> this cookie has nothing specific to iopf.
>
> it may makes more sense to build a generic iommu_attach_device_cookie()
> helper so the same object can be reused in future other usages too.
>
> within iommu core it can check domain iopf handler and this generic cookie
> to update iopf specific data e.g. the pasid_cookie xarray.
This means attaching an iopf-capable domain follows two steps:
1) Attaching the domain to the device.
2) Setting up the iopf data, necessary for handling iopf data.
This creates a time window during which the iopf is enabled, but the
software cannot handle it. Or not?
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Wednesday, February 21, 2024 1:53 PM
>
> On 2024/2/7 16:11, Tian, Kevin wrote:
> >> From: Lu Baolu <[email protected]>
> >> Sent: Monday, January 22, 2024 3:39 PM
> >>
> >> There is a slight difference between iopf domains and non-iopf domains.
> >> In the latter, references to domains occur between attach and detach;
> >> While in the former, due to the existence of asynchronous iopf handling
> >> paths, references to the domain may occur after detach, which leads to
> >> potential UAF issues.
> >
> > Does UAF still exist if iommu driver follows the guidance you just added
> > to iopf_queue_remove_device()?
> >
> > it clearly says that the driver needs to disable IOMMU PRI reception,
> > remove device from iopf queue and disable PRI on the device.
>
> The iopf_queue_remove_device() function is only called after the last
> iopf-capable domain is detached from the device. It may not be called
> during domain replacement. Hence, there is no guarantee that
> iopf_queue_remove_device() will be called when a domain is detached from
> the device.
oh yes. More accurately even the last detach may not trigger it.
e.g. idxd driver does it at device/driver unbind.
>
> >
> > presumably those are all about what needs to be done in the detach
> > operation. Then once detach completes there should be no more
> > reference to the domain from the iopf path?
>
> The domain pointer stored in the iopf_group structure is only released
> after the iopf response, possibly after the domain is detached from the
> device. Thus, the domain pointer can only be freed after the iopf
> response.
make sense.
>
> >
> >>
> >> +struct iopf_attach_cookie {
> >> + struct iommu_domain *domain;
> >> + struct device *dev;
> >> + unsigned int pasid;
> >> + refcount_t users;
> >> +
> >> + void *private;
> >> + void (*release)(struct iopf_attach_cookie *cookie);
> >> +};
> >
> > this cookie has nothing specific to iopf.
> >
> > it may makes more sense to build a generic iommu_attach_device_cookie()
> > helper so the same object can be reused in future other usages too.
> >
> > within iommu core it can check domain iopf handler and this generic cookie
> > to update iopf specific data e.g. the pasid_cookie xarray.
>
> This means attaching an iopf-capable domain follows two steps:
>
> 1) Attaching the domain to the device.
> 2) Setting up the iopf data, necessary for handling iopf data.
>
> This creates a time window during which the iopf is enabled, but the
> software cannot handle it. Or not?
>
why two steps? in attach you can setup the iopf data when recognizing
that the domain is iopf capable...
On 2024/2/21 14:49, Tian, Kevin wrote:
>>>> +struct iopf_attach_cookie {
>>>> + struct iommu_domain *domain;
>>>> + struct device *dev;
>>>> + unsigned int pasid;
>>>> + refcount_t users;
>>>> +
>>>> + void *private;
>>>> + void (*release)(struct iopf_attach_cookie *cookie);
>>>> +};
>>> this cookie has nothing specific to iopf.
>>>
>>> it may makes more sense to build a generic iommu_attach_device_cookie()
>>> helper so the same object can be reused in future other usages too.
>>>
>>> within iommu core it can check domain iopf handler and this generic cookie
>>> to update iopf specific data e.g. the pasid_cookie xarray.
>> This means attaching an iopf-capable domain follows two steps:
>>
>> 1) Attaching the domain to the device.
>> 2) Setting up the iopf data, necessary for handling iopf data.
>>
>> This creates a time window during which the iopf is enabled, but the
>> software cannot handle it. Or not?
>>
> why two steps? in attach you can setup the iopf data when recognizing
> that the domain is iopf capable...
Oh, maybe I misunderstood. So your proposal is to make the new interface
generic, not for iopf only?
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Wednesday, February 21, 2024 3:21 PM
>
> On 2024/2/21 14:49, Tian, Kevin wrote:
> >>>> +struct iopf_attach_cookie {
> >>>> + struct iommu_domain *domain;
> >>>> + struct device *dev;
> >>>> + unsigned int pasid;
> >>>> + refcount_t users;
> >>>> +
> >>>> + void *private;
> >>>> + void (*release)(struct iopf_attach_cookie *cookie);
> >>>> +};
> >>> this cookie has nothing specific to iopf.
> >>>
> >>> it may makes more sense to build a generic
> iommu_attach_device_cookie()
> >>> helper so the same object can be reused in future other usages too.
> >>>
> >>> within iommu core it can check domain iopf handler and this generic
> cookie
> >>> to update iopf specific data e.g. the pasid_cookie xarray.
> >> This means attaching an iopf-capable domain follows two steps:
> >>
> >> 1) Attaching the domain to the device.
> >> 2) Setting up the iopf data, necessary for handling iopf data.
> >>
> >> This creates a time window during which the iopf is enabled, but the
> >> software cannot handle it. Or not?
> >>
> > why two steps? in attach you can setup the iopf data when recognizing
> > that the domain is iopf capable...
>
> Oh, maybe I misunderstood. So your proposal is to make the new interface
> generic, not for iopf only?
>
yes.