| Message ID | 20180920170046.20154-3-jean-philippe.brucker@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Shared Virtual Addressing for the IOMMU | expand |
Hi, On 09/21/2018 01:00 AM, Jean-Philippe Brucker wrote: > Add bind() and unbind() operations to the IOMMU API. Bind() returns a > PASID that drivers can program in hardware, to let their devices access an > mm. This patch only adds skeletons for the device driver API, most of the > implementation is still missing. Is it possible that a malicious process can unbind a pasid which is used by another normal process? It might happen in below sequence: Process A Process B ========= ========= iommu_sva_init_device(dev) iommu_sva_bind_device(dev) .... device access mm of A with #PASID returned above .... iommu_sva_unbind_device(dev, #PASID) .... [unrecoverable errors] I didn't have a thorough consideration of this. Sorry if this has been prevented. Best regards, Lu Baolu > > Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com> > --- > drivers/iommu/iommu-sva.c | 34 +++++++++++++++ > drivers/iommu/iommu.c | 90 +++++++++++++++++++++++++++++++++++++++ > include/linux/iommu.h | 37 ++++++++++++++++ > 3 files changed, 161 insertions(+) > > diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c > index 85ef98efede8..d60d4f0bb89e 100644 > --- a/drivers/iommu/iommu-sva.c > +++ b/drivers/iommu/iommu-sva.c > @@ -8,6 +8,38 @@ > #include <linux/iommu.h> > #include <linux/slab.h> > > +int __iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, int *pasid, > + unsigned long flags, void *drvdata) > +{ > + return -ENOSYS; /* TODO */ > +} > +EXPORT_SYMBOL_GPL(__iommu_sva_bind_device); > + > +int __iommu_sva_unbind_device(struct device *dev, int pasid) > +{ > + return -ENOSYS; /* TODO */ > +} > +EXPORT_SYMBOL_GPL(__iommu_sva_unbind_device); > + > +static void __iommu_sva_unbind_device_all(struct device *dev) > +{ > + /* TODO */ > +} > + > +/** > + * iommu_sva_unbind_device_all() - Detach all address spaces from this device > + * @dev: the device > + * > + * When detaching @dev from a domain, IOMMU drivers should use this helper. > + */ > +void iommu_sva_unbind_device_all(struct device *dev) > +{ > + mutex_lock(&dev->iommu_param->sva_lock); > + __iommu_sva_unbind_device_all(dev); > + mutex_unlock(&dev->iommu_param->sva_lock); > +} > +EXPORT_SYMBOL_GPL(iommu_sva_unbind_device_all); > + > /** > * iommu_sva_init_device() - Initialize Shared Virtual Addressing for a device > * @dev: the device > @@ -96,6 +128,8 @@ void iommu_sva_shutdown_device(struct device *dev) > if (!param) > goto out_unlock; > > + __iommu_sva_unbind_device_all(dev); > + > if (domain->ops->sva_shutdown_device) > domain->ops->sva_shutdown_device(dev); > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index fa0561ed006f..aba3bf15d46c 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -2325,3 +2325,93 @@ int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids) > return 0; > } > EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids); > + > +/** > + * iommu_sva_bind_device() - Bind a process address space to a device > + * @dev: the device > + * @mm: the mm to bind, caller must hold a reference to it > + * @pasid: valid address where the PASID will be stored > + * @flags: bond properties > + * @drvdata: private data passed to the mm exit handler > + * > + * Create a bond between device and task, allowing the device to access the mm > + * using the returned PASID. If unbind() isn't called first, a subsequent bind() > + * for the same device and mm fails with -EEXIST. > + * > + * iommu_sva_init_device() must be called first, to initialize the required SVA > + * features. @flags must be a subset of these features. > + * > + * The caller must pin down using get_user_pages*() all mappings shared with the > + * device. mlock() isn't sufficient, as it doesn't prevent minor page faults > + * (e.g. copy-on-write). > + * > + * On success, 0 is returned and @pasid contains a valid ID. Otherwise, an error > + * is returned. > + */ > +int iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, int *pasid, > + unsigned long flags, void *drvdata) > +{ > + int ret = -EINVAL; > + struct iommu_group *group; > + > + if (!pasid) > + return -EINVAL; > + > + group = iommu_group_get(dev); > + if (!group) > + return -ENODEV; > + > + /* Ensure device count and domain don't change while we're binding */ > + mutex_lock(&group->mutex); > + > + /* > + * To keep things simple, SVA currently doesn't support IOMMU groups > + * with more than one device. Existing SVA-capable systems are not > + * affected by the problems that required IOMMU groups (lack of ACS > + * isolation, device ID aliasing and other hardware issues). > + */ > + if (iommu_group_device_count(group) != 1) > + goto out_unlock; > + > + ret = __iommu_sva_bind_device(dev, mm, pasid, flags, drvdata); > + > +out_unlock: > + mutex_unlock(&group->mutex); > + iommu_group_put(group); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(iommu_sva_bind_device); > + > +/** > + * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device > + * @dev: the device > + * @pasid: the pasid returned by bind() > + * > + * Remove bond between device and address space identified by @pasid. Users > + * should not call unbind() if the corresponding mm exited (as the PASID might > + * have been reallocated for another process). > + * > + * The device must not be issuing any more transaction for this PASID. All > + * outstanding page requests for this PASID must have been flushed to the IOMMU. > + * > + * Returns 0 on success, or an error value > + */ > +int iommu_sva_unbind_device(struct device *dev, int pasid) > +{ > + int ret = -EINVAL; > + struct iommu_group *group; > + > + group = iommu_group_get(dev); > + if (!group) > + return -ENODEV; > + > + mutex_lock(&group->mutex); > + ret = __iommu_sva_unbind_device(dev, pasid); > + mutex_unlock(&group->mutex); > + > + iommu_group_put(group); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(iommu_sva_unbind_device); > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 4c27cb347770..9c49877e37a5 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -586,6 +586,10 @@ void iommu_fwspec_free(struct device *dev); > int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids); > const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode); > > +extern int iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, > + int *pasid, unsigned long flags, void *drvdata); > +extern int iommu_sva_unbind_device(struct device *dev, int pasid); > + > #else /* CONFIG_IOMMU_API */ > > struct iommu_ops {}; > @@ -910,6 +914,18 @@ static inline int iommu_sva_invalidate(struct iommu_domain *domain, > return -ENODEV; > } > > +static inline int iommu_sva_bind_device(struct device *dev, > + struct mm_struct *mm, int *pasid, > + unsigned long flags, void *drvdata) > +{ > + return -ENODEV; > +} > + > +static inline int iommu_sva_unbind_device(struct device *dev, int pasid) > +{ > + return -ENODEV; > +} > + > #endif /* CONFIG_IOMMU_API */ > > #ifdef CONFIG_IOMMU_DEBUGFS > @@ -924,6 +940,11 @@ extern int iommu_sva_init_device(struct device *dev, unsigned long features, > unsigned int min_pasid, > unsigned int max_pasid); > extern void iommu_sva_shutdown_device(struct device *dev); > +extern int __iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, > + int *pasid, unsigned long flags, > + void *drvdata); > +extern int __iommu_sva_unbind_device(struct device *dev, int pasid); > +extern void iommu_sva_unbind_device_all(struct device *dev); > #else /* CONFIG_IOMMU_SVA */ > static inline int iommu_sva_init_device(struct device *dev, > unsigned long features, > @@ -936,6 +957,22 @@ static inline int iommu_sva_init_device(struct device *dev, > static inline void iommu_sva_shutdown_device(struct device *dev) > { > } > + > +static inline int __iommu_sva_bind_device(struct device *dev, > + struct mm_struct *mm, int *pasid, > + unsigned long flags, void *drvdata) > +{ > + return -ENODEV; > +} > + > +static inline int __iommu_sva_unbind_device(struct device *dev, int pasid) > +{ > + return -ENODEV; > +} > + > +static inline void iommu_sva_unbind_device_all(struct device *dev) > +{ > +} > #endif /* CONFIG_IOMMU_SVA */ > > #endif /* __LINUX_IOMMU_H */ >
On 23/09/2018 04:05, Lu Baolu wrote: > Hi, > > On 09/21/2018 01:00 AM, Jean-Philippe Brucker wrote: >> Add bind() and unbind() operations to the IOMMU API. Bind() returns a >> PASID that drivers can program in hardware, to let their devices access an >> mm. This patch only adds skeletons for the device driver API, most of the >> implementation is still missing. > > Is it possible that a malicious process can unbind a pasid which is > used by another normal process? Yes, it's up to the device driver that calls unbind() to check that the caller is allowed to unbind this PASID. We can't do it ourselves since unbind() could also be called from a kernel thread for example from a cleanup function in some workqueue, outside the context of the process to unbind. Thanks, Jean > > It might happen in below sequence: > > > Process A Process B > ========= ========= > iommu_sva_init_device(dev) > iommu_sva_bind_device(dev) > .... > device access mm of A with > #PASID returned above > .... > iommu_sva_unbind_device(dev, #PASID) > .... > [unrecoverable errors] > > I didn't have a thorough consideration of this. Sorry if this has been > prevented. > > Best regards, > Lu Baolu
On Mon, 24 Sep 2018 13:07:47 +0100 Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote: > On 23/09/2018 04:05, Lu Baolu wrote: > > Hi, > > > > On 09/21/2018 01:00 AM, Jean-Philippe Brucker wrote: > >> Add bind() and unbind() operations to the IOMMU API. Bind() > >> returns a PASID that drivers can program in hardware, to let their > >> devices access an mm. This patch only adds skeletons for the > >> device driver API, most of the implementation is still missing. > > > > Is it possible that a malicious process can unbind a pasid which is > > used by another normal process? > > Yes, it's up to the device driver that calls unbind() to check that > the caller is allowed to unbind this PASID. We can't do it ourselves > since unbind() could also be called from a kernel thread for example > from a cleanup function in some workqueue, outside the context of the > process to unbind. > I am wondering if we can avoid the complexity around permission checking by simply _only_ allow bind/unbind() on current mm? what would be the missing use cases if we bind current only? It can also avoid other race such as unbind and mmu_notifier release call. > Jean > > > > > It might happen in below sequence: > > > > > > Process A Process B > > ========= ========= > > iommu_sva_init_device(dev) > > iommu_sva_bind_device(dev) > > .... > > device access mm of A with > > #PASID returned above > > .... > > iommu_sva_unbind_device(dev, #PASID) > > .... > > [unrecoverable errors] > > > > I didn't have a thorough consideration of this. Sorry if this has > > been prevented. > > > > Best regards, > > Lu Baolu [Jacob Pan]
On 26/09/2018 19:01, Jacob Pan wrote: > On Mon, 24 Sep 2018 13:07:47 +0100 > Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote: > >> On 23/09/2018 04:05, Lu Baolu wrote: >> > Hi, >> > >> > On 09/21/2018 01:00 AM, Jean-Philippe Brucker wrote: >> >> Add bind() and unbind() operations to the IOMMU API. Bind() >> >> returns a PASID that drivers can program in hardware, to let their >> >> devices access an mm. This patch only adds skeletons for the >> >> device driver API, most of the implementation is still missing. >> > >> > Is it possible that a malicious process can unbind a pasid which is >> > used by another normal process? >> >> Yes, it's up to the device driver that calls unbind() to check that >> the caller is allowed to unbind this PASID. We can't do it ourselves >> since unbind() could also be called from a kernel thread for example >> from a cleanup function in some workqueue, outside the context of the >> process to unbind. Actually I'm not too concerned about a process unbinding another one, since in general only the kernel will hold the PASID values. Userspace shouldn't even need to see them, so issuing unbind() with the wrong PASID isn't an easy mistake. > I am wondering if we can avoid the complexity around permission > checking by simply _only_ allow bind/unbind() on current mm? what would > be the missing use cases if we bind current only? > It can also avoid other race such as unbind and mmu_notifier release > call. That's tempting but may be too restrictive. I just tried to copy what the current AMD and Intel drivers do in their SVA implementation, but I don't know if users will need all of it. At the moment the amdkfd driver does unbind() from a workqueue, although moving to the generic API might simplify things there. Callers can easily enforce that only current->mm is passed to bind(). I don't know if allowing a process to bind another one is a real use-case, but the permission check on the device driver side is fairly easy, and disallowing it wouldn't simplify iommu-sva. Even if we allow bind() only on current, forcing unbind() to be done on current means that the driver can't clean things up from a workqueue. But you're right that this restriction would make things *much* simpler for the exit()/unbind() race. Thanks, Jean
> From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@arm.com] > Sent: Thursday, September 27, 2018 11:06 PM > > On 26/09/2018 19:01, Jacob Pan wrote: > > On Mon, 24 Sep 2018 13:07:47 +0100 > > Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote: > > > >> On 23/09/2018 04:05, Lu Baolu wrote: > >> > Hi, > >> > > >> > On 09/21/2018 01:00 AM, Jean-Philippe Brucker wrote: > >> >> Add bind() and unbind() operations to the IOMMU API. Bind() > >> >> returns a PASID that drivers can program in hardware, to let their > >> >> devices access an mm. This patch only adds skeletons for the > >> >> device driver API, most of the implementation is still missing. > >> > > >> > Is it possible that a malicious process can unbind a pasid which is > >> > used by another normal process? > >> > >> Yes, it's up to the device driver that calls unbind() to check that > >> the caller is allowed to unbind this PASID. We can't do it ourselves > >> since unbind() could also be called from a kernel thread for example > >> from a cleanup function in some workqueue, outside the context of the > >> process to unbind. > > Actually I'm not too concerned about a process unbinding another one, > since in general only the kernel will hold the PASID values. Userspace > shouldn't even need to see them, so issuing unbind() with the wrong > PASID isn't an easy mistake. > well, it depends on which scenario is talked here. for native SVA with device driver in kernel, your description is correct. for native SVA with device driver in user space, then user space needs to see/hold PASIDs and program them to device specific register. for virtual SVA (vtd case), Qemu needs to see/hold PASIDs and pass to guest upon any PASID allocation request thru a PV channel, as you just saw in another thread. :-) Thanks Kevin
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c index 85ef98efede8..d60d4f0bb89e 100644 --- a/drivers/iommu/iommu-sva.c +++ b/drivers/iommu/iommu-sva.c @@ -8,6 +8,38 @@ #include <linux/iommu.h> #include <linux/slab.h> +int __iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, int *pasid, + unsigned long flags, void *drvdata) +{ + return -ENOSYS; /* TODO */ +} +EXPORT_SYMBOL_GPL(__iommu_sva_bind_device); + +int __iommu_sva_unbind_device(struct device *dev, int pasid) +{ + return -ENOSYS; /* TODO */ +} +EXPORT_SYMBOL_GPL(__iommu_sva_unbind_device); + +static void __iommu_sva_unbind_device_all(struct device *dev) +{ + /* TODO */ +} + +/** + * iommu_sva_unbind_device_all() - Detach all address spaces from this device + * @dev: the device + * + * When detaching @dev from a domain, IOMMU drivers should use this helper. + */ +void iommu_sva_unbind_device_all(struct device *dev) +{ + mutex_lock(&dev->iommu_param->sva_lock); + __iommu_sva_unbind_device_all(dev); + mutex_unlock(&dev->iommu_param->sva_lock); +} +EXPORT_SYMBOL_GPL(iommu_sva_unbind_device_all); + /** * iommu_sva_init_device() - Initialize Shared Virtual Addressing for a device * @dev: the device @@ -96,6 +128,8 @@ void iommu_sva_shutdown_device(struct device *dev) if (!param) goto out_unlock; + __iommu_sva_unbind_device_all(dev); + if (domain->ops->sva_shutdown_device) domain->ops->sva_shutdown_device(dev); diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index fa0561ed006f..aba3bf15d46c 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2325,3 +2325,93 @@ int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids) return 0; } EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids); + +/** + * iommu_sva_bind_device() - Bind a process address space to a device + * @dev: the device + * @mm: the mm to bind, caller must hold a reference to it + * @pasid: valid address where the PASID will be stored + * @flags: bond properties + * @drvdata: private data passed to the mm exit handler + * + * Create a bond between device and task, allowing the device to access the mm + * using the returned PASID. If unbind() isn't called first, a subsequent bind() + * for the same device and mm fails with -EEXIST. + * + * iommu_sva_init_device() must be called first, to initialize the required SVA + * features. @flags must be a subset of these features. + * + * The caller must pin down using get_user_pages*() all mappings shared with the + * device. mlock() isn't sufficient, as it doesn't prevent minor page faults + * (e.g. copy-on-write). + * + * On success, 0 is returned and @pasid contains a valid ID. Otherwise, an error + * is returned. + */ +int iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, int *pasid, + unsigned long flags, void *drvdata) +{ + int ret = -EINVAL; + struct iommu_group *group; + + if (!pasid) + return -EINVAL; + + group = iommu_group_get(dev); + if (!group) + return -ENODEV; + + /* Ensure device count and domain don't change while we're binding */ + mutex_lock(&group->mutex); + + /* + * To keep things simple, SVA currently doesn't support IOMMU groups + * with more than one device. Existing SVA-capable systems are not + * affected by the problems that required IOMMU groups (lack of ACS + * isolation, device ID aliasing and other hardware issues). + */ + if (iommu_group_device_count(group) != 1) + goto out_unlock; + + ret = __iommu_sva_bind_device(dev, mm, pasid, flags, drvdata); + +out_unlock: + mutex_unlock(&group->mutex); + iommu_group_put(group); + + return ret; +} +EXPORT_SYMBOL_GPL(iommu_sva_bind_device); + +/** + * iommu_sva_unbind_device() - Remove a bond created with iommu_sva_bind_device + * @dev: the device + * @pasid: the pasid returned by bind() + * + * Remove bond between device and address space identified by @pasid. Users + * should not call unbind() if the corresponding mm exited (as the PASID might + * have been reallocated for another process). + * + * The device must not be issuing any more transaction for this PASID. All + * outstanding page requests for this PASID must have been flushed to the IOMMU. + * + * Returns 0 on success, or an error value + */ +int iommu_sva_unbind_device(struct device *dev, int pasid) +{ + int ret = -EINVAL; + struct iommu_group *group; + + group = iommu_group_get(dev); + if (!group) + return -ENODEV; + + mutex_lock(&group->mutex); + ret = __iommu_sva_unbind_device(dev, pasid); + mutex_unlock(&group->mutex); + + iommu_group_put(group); + + return ret; +} +EXPORT_SYMBOL_GPL(iommu_sva_unbind_device); diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 4c27cb347770..9c49877e37a5 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -586,6 +586,10 @@ void iommu_fwspec_free(struct device *dev); int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids); const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode); +extern int iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, + int *pasid, unsigned long flags, void *drvdata); +extern int iommu_sva_unbind_device(struct device *dev, int pasid); + #else /* CONFIG_IOMMU_API */ struct iommu_ops {}; @@ -910,6 +914,18 @@ static inline int iommu_sva_invalidate(struct iommu_domain *domain, return -ENODEV; } +static inline int iommu_sva_bind_device(struct device *dev, + struct mm_struct *mm, int *pasid, + unsigned long flags, void *drvdata) +{ + return -ENODEV; +} + +static inline int iommu_sva_unbind_device(struct device *dev, int pasid) +{ + return -ENODEV; +} + #endif /* CONFIG_IOMMU_API */ #ifdef CONFIG_IOMMU_DEBUGFS @@ -924,6 +940,11 @@ extern int iommu_sva_init_device(struct device *dev, unsigned long features, unsigned int min_pasid, unsigned int max_pasid); extern void iommu_sva_shutdown_device(struct device *dev); +extern int __iommu_sva_bind_device(struct device *dev, struct mm_struct *mm, + int *pasid, unsigned long flags, + void *drvdata); +extern int __iommu_sva_unbind_device(struct device *dev, int pasid); +extern void iommu_sva_unbind_device_all(struct device *dev); #else /* CONFIG_IOMMU_SVA */ static inline int iommu_sva_init_device(struct device *dev, unsigned long features, @@ -936,6 +957,22 @@ static inline int iommu_sva_init_device(struct device *dev, static inline void iommu_sva_shutdown_device(struct device *dev) { } + +static inline int __iommu_sva_bind_device(struct device *dev, + struct mm_struct *mm, int *pasid, + unsigned long flags, void *drvdata) +{ + return -ENODEV; +} + +static inline int __iommu_sva_unbind_device(struct device *dev, int pasid) +{ + return -ENODEV; +} + +static inline void iommu_sva_unbind_device_all(struct device *dev) +{ +} #endif /* CONFIG_IOMMU_SVA */ #endif /* __LINUX_IOMMU_H */
Add bind() and unbind() operations to the IOMMU API. Bind() returns a PASID that drivers can program in hardware, to let their devices access an mm. This patch only adds skeletons for the device driver API, most of the implementation is still missing. Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com> --- drivers/iommu/iommu-sva.c | 34 +++++++++++++++ drivers/iommu/iommu.c | 90 +++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h | 37 ++++++++++++++++ 3 files changed, 161 insertions(+)