| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 18 Nov 2021 10:10:43 -0400
From: Jason Gunthorpe <jgg@...dia.com>
To: Lu Baolu <baolu.lu@...ux.intel.com>
Cc: Christoph Hellwig <hch@...radead.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Joerg Roedel <joro@...tes.org>,
Alex Williamson <alex.williamson@...hat.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
Kevin Tian <kevin.tian@...el.com>,
Ashok Raj <ashok.raj@...el.com>,
Chaitanya Kulkarni <kch@...dia.com>, kvm@...r.kernel.org,
rafael@...nel.org, linux-pci@...r.kernel.org,
Cornelia Huck <cohuck@...hat.com>,
linux-kernel@...r.kernel.org, iommu@...ts.linux-foundation.org,
Jacob jun Pan <jacob.jun.pan@...el.com>,
Diana Craciun <diana.craciun@....nxp.com>,
Will Deacon <will@...nel.org>
Subject: Re: [PATCH 01/11] iommu: Add device dma ownership set/release
interfaces
On Thu, Nov 18, 2021 at 09:12:41AM +0800, Lu Baolu wrote:
> The existing iommu_attach_device() allows only for singleton group. As
> we have added group ownership attribute, we can enforce this interface
> only for kernel domain usage.
Below is what I came up with.
- Replace the file * with a simple void *
- Use owner_count == 0 <-> dma_owner == DMA_OWNER to simplify
the logic and remove levels of indent
- Add a kernel state DMA_OWNER_PRIVATE_DOMAIN
- Rename the user state to DMA_OWNER_PRIVATE_DOMAIN_USER
It differs from the above because it does extra work to keep the
group isolated that kernel users do no need to do.
- Rename the kernel state to DMA_OWNER_DMA_API to better reflect
its purpose. Inspired by Robin's point that alot of this is
indirectly coupled to the domain pointer.
- Have iommu_attach_device() atomically swap from DMA_OWNER_DMA_API
to DMA_OWNER_PRIVATE_DOMAIN - replaces the group size check.
When we figure out tegra we can add an WARN_ON to iommu_attach_group()
that dma_owner != DMA_OWNER_NONE || DMA_OWNER_DMA_API
Then the whole thing makes some general sense..
Jason
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 064d0679906afd..4cafe074775e30 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -49,7 +49,7 @@ struct iommu_group {
struct list_head entry;
enum iommu_dma_owner dma_owner;
refcount_t owner_cnt;
- struct file *owner_user_file;
+ void *owner_cookie;
};
struct group_device {
@@ -1937,12 +1937,18 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
* change while we are attaching
*/
mutex_lock(&group->mutex);
- ret = -EINVAL;
- if (iommu_group_device_count(group) != 1)
+ if (group->dma_owner != DMA_OWNER_DMA_API ||
+ refcount_read(&group->owner_cnt) != 1) {
+ ret = -EBUSY;
goto out_unlock;
+ }
ret = __iommu_attach_group(domain, group);
+ if (ret)
+ goto out_unlock;
+ group->dma_owner = DMA_OWNER_PRIVATE_DOMAIN;
+ group->owner_cookie = domain;
out_unlock:
mutex_unlock(&group->mutex);
iommu_group_put(group);
@@ -2193,14 +2199,11 @@ void iommu_detach_device(struct iommu_domain *domain, struct device *dev)
return;
mutex_lock(&group->mutex);
- if (iommu_group_device_count(group) != 1) {
- WARN_ON(1);
- goto out_unlock;
- }
-
+ WARN_ON(group->dma_owner != DMA_OWNER_PRIVATE_DOMAIN ||
+ refcount_read(&group->owner_cnt) != 1 ||
+ group->owner_cookie != domain);
+ group->dma_owner = DMA_OWNER_DMA_API;
__iommu_detach_group(domain, group);
-
-out_unlock:
mutex_unlock(&group->mutex);
iommu_group_put(group);
}
@@ -3292,44 +3295,33 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
static int __iommu_group_set_dma_owner(struct iommu_group *group,
enum iommu_dma_owner owner,
- struct file *user_file)
+ void *owner_cookie)
{
- if (group->dma_owner != DMA_OWNER_NONE && group->dma_owner != owner)
- return -EBUSY;
-
- if (owner == DMA_OWNER_USER) {
- if (!user_file)
- return -EINVAL;
-
- if (group->owner_user_file && group->owner_user_file != user_file)
- return -EPERM;
+ if (refcount_inc_not_zero(&group->owner_cnt)) {
+ if (group->dma_owner != owner ||
+ group->owner_cookie != owner_cookie) {
+ refcount_dec(&group->owner_cnt);
+ return -EBUSY;
+ }
+ return 0;
}
- if (!refcount_inc_not_zero(&group->owner_cnt)) {
- group->dma_owner = owner;
- refcount_set(&group->owner_cnt, 1);
-
- if (owner == DMA_OWNER_USER) {
- /*
- * The UNMANAGED domain shouldn't be attached before
- * claiming the USER ownership for the first time.
- */
- if (group->domain) {
- if (group->domain != group->default_domain) {
- group->dma_owner = DMA_OWNER_NONE;
- refcount_set(&group->owner_cnt, 0);
-
- return -EBUSY;
- }
-
- __iommu_detach_group(group->domain, group);
- }
-
- get_file(user_file);
- group->owner_user_file = user_file;
+ /*
+ * We must ensure that any device DMAs issued after this call
+ * are discarded. DMAs can only reach real memory once someone
+ * has attached a real domain.
+ */
+ if (owner == DMA_OWNER_PRIVATE_DOMAIN_USER) {
+ if (group->domain) {
+ if (group->domain != group->default_domain)
+ return -EBUSY;
+ __iommu_detach_group(group->domain, group);
}
}
+ group->dma_owner = owner;
+ group->owner_cookie = owner_cookie;
+ refcount_set(&group->owner_cnt, 1);
return 0;
}
@@ -3339,20 +3331,18 @@ static void __iommu_group_release_dma_owner(struct iommu_group *group,
if (WARN_ON(group->dma_owner != owner))
return;
- if (refcount_dec_and_test(&group->owner_cnt)) {
- group->dma_owner = DMA_OWNER_NONE;
+ if (!refcount_dec_and_test(&group->owner_cnt))
+ return;
- if (owner == DMA_OWNER_USER) {
- fput(group->owner_user_file);
- group->owner_user_file = NULL;
+ group->dma_owner = DMA_OWNER_NONE;
- /*
- * The UNMANAGED domain should be detached before all USER
- * owners have been released.
- */
- if (!WARN_ON(group->domain) && group->default_domain)
- __iommu_attach_group(group->default_domain, group);
- }
+ /*
+ * The UNMANAGED domain should be detached before all USER
+ * owners have been released.
+ */
+ if (owner == DMA_OWNER_PRIVATE_DOMAIN_USER) {
+ if (!WARN_ON(group->domain) && group->default_domain)
+ __iommu_attach_group(group->default_domain, group);
}
}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d8946f22edd5df..7f50dfa7207e9c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -164,14 +164,21 @@ enum iommu_dev_features {
/**
* enum iommu_dma_owner - IOMMU DMA ownership
- * @DMA_OWNER_NONE: No DMA ownership
- * @DMA_OWNER_KERNEL: Device DMAs are initiated by a kernel driver
- * @DMA_OWNER_USER: Device DMAs are initiated by a userspace driver
+ * @DMA_OWNER_NONE:
+ * No DMA ownership
+ * @DMA_OWNER_DMA_API:
+ * Device DMAs are initiated by a kernel driver through the DMA API
+ * @DMA_OWNER_PRIVATE_DOMAIN:
+ * Device DMAs are initiated by a kernel driver
+ * @DMA_OWNER_PRIVATE_DOMAIN_USER:
+ * Device DMAs are initiated by userspace, kernel ensures that DMAs
+ * never go to kernel memory.
*/
enum iommu_dma_owner {
DMA_OWNER_NONE,
- DMA_OWNER_KERNEL,
- DMA_OWNER_USER,
+ DMA_OWNER_DMA_API,
+ DMA_OWNER_PRIVATE_DOMAIN,
+ DMA_OWNER_PRIVATE_DOMAIN_USER,
};
#define IOMMU_PASID_INVALID (-1U)
Powered by blists - more mailing lists