Re: [bug] NULL pointer deref after 3f6634d997db ("iommu: Use right way to retrieve iommu_ops")

[Jan Stancek]

On Wed, May 4, 2022 at 1:14 PM Robin Murphy  wrote:
>
> On 2022-05-04 08:53, Jan Stancek wrote:
> [...]
> > Hi,
> >
> > I'm getting panics after hunk above was applied in this patch
> > on ppc64le KVM guest, dev->iommu is NULL.
>
> Oof, this can probably be hit with vfio-noiommu too, and by the look of
> things, `echo auto > /sys/kernel/iommu_groups/0/type` would likely blow
> up as well. Does the patch below work for you?

Thanks for quick reply. Yes, it does.

# cat /sys/kernel/iommu_groups/0/reserved_regions
# echo auto > /sys/kernel/iommu_groups/0/type
-bash: echo: write error: Invalid argument

Tested-by: Jan Stancek 

>
> Thanks,
> Robin.
>
> ->8-
>  From abf0a38563bb2922a849e235d33d342170b5bc90 Mon Sep 17 00:00:00 2001
> Message-Id: 
> 
> From: Robin Murphy 
> Date: Wed, 4 May 2022 11:53:20 +0100
> Subject: [PATCH] iommu: Make sysfs robust for non-API groups
>
> Groups created by VFIO backends outside the core IOMMU API should never
> be passed directly into the API itself, however they still expose their
> standard sysfs attributes, so we can still stumble across them that way.
> Take care to consider those cases before jumping into our normal
> assumptions of a fully-initialised core API group.
>
> Fixes: 3f6634d997db ("iommu: Use right way to retrieve iommu_ops")
> Reported-by: Jan Stancek 
> Signed-off-by: Robin Murphy 
> ---
>   drivers/iommu/iommu.c | 9 -
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 29906bc16371..41ea2deaee03 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -510,6 +510,13 @@ int iommu_get_group_resv_regions(struct iommu_group 
> *group,
> list_for_each_entry(device, >devices, list) {
> struct list_head dev_resv_regions;
>
> +   /*
> +* Non-API groups still expose reserved_regions in sysfs,
> +* so filter out calls that get here that way.
> +*/
> +   if (!device->dev->iommu)
> +   break;
> +
> INIT_LIST_HEAD(_resv_regions);
> iommu_get_resv_regions(device->dev, _resv_regions);
> ret = iommu_insert_device_resv_regions(_resv_regions, 
> head);
> @@ -2977,7 +2984,7 @@ static ssize_t iommu_group_store_type(struct 
> iommu_group *group,
> if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> return -EACCES;
>
> -   if (WARN_ON(!group))
> +   if (WARN_ON(!group) || !group->default_domain)
> return -EINVAL;
>
> if (sysfs_streq(buf, "identity"))
> --
> 2.35.3.dirty
>

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[bug] NULL pointer deref after 3f6634d997db ("iommu: Use right way to retrieve iommu_ops")

[Jan Stancek]

On Wed, Feb 16, 2022 at 10:52:47AM +0800, Lu Baolu wrote:

The common iommu_ops is hooked to both device and domain. When a helper
has both device and domain pointer, the way to get the iommu_ops looks
messy in iommu core. This sorts out the way to get iommu_ops. The device
related helpers go through device pointer, while the domain related ones
go through domain pointer.

Signed-off-by: Lu Baolu 
Reviewed-by: Christoph Hellwig 
Reviewed-by: Jason Gunthorpe 
---
include/linux/iommu.h | 11 ++
drivers/iommu/iommu.c | 50 +--
2 files changed, 36 insertions(+), 25 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9ffa2e88f3d0..90f1b5e3809d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -381,6 +381,17 @@ static inline void iommu_iotlb_gather_init(struct 
iommu_iotlb_gather *gather)
};
}

+static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
+{
+   /*
+* Assume that valid ops must be installed if iommu_probe_device()
+* has succeeded. The device ops are essentially for internal use
+* within the IOMMU subsystem itself, so we should be able to trust
+* ourselves not to misuse the helper.
+*/
+   return dev->iommu->iommu_dev->ops;
+}
+
#define IOMMU_GROUP_NOTIFY_ADD_DEVICE   1 /* Device added */
#define IOMMU_GROUP_NOTIFY_DEL_DEVICE   2 /* Pre Device removed */
#define IOMMU_GROUP_NOTIFY_BIND_DRIVER  3 /* Pre Driver bind */
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7cf073c56036..7af0ee670deb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -323,13 +323,14 @@ int iommu_probe_device(struct device *dev)

void iommu_release_device(struct device *dev)
{
-   const struct iommu_ops *ops = dev->bus->iommu_ops;
+   const struct iommu_ops *ops;

if (!dev->iommu)
return;

iommu_device_unlink(dev->iommu->iommu_dev, dev);

+   ops = dev_iommu_ops(dev);
ops->release_device(dev);

iommu_group_remove_device(dev);
@@ -833,8 +834,10 @@ static int iommu_create_device_direct_mappings(struct 
iommu_group *group,
static bool iommu_is_attach_deferred(struct iommu_domain *domain,
 struct device *dev)
{
-   if (domain->ops->is_attach_deferred)
-   return domain->ops->is_attach_deferred(domain, dev);
+   const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+   if (ops->is_attach_deferred)
+   return ops->is_attach_deferred(domain, dev);

return false;
}
@@ -1252,10 +1255,10 @@ int iommu_page_response(struct device *dev,
struct iommu_fault_event *evt;
struct iommu_fault_page_request *prm;
struct dev_iommu *param = dev->iommu;
+   const struct iommu_ops *ops = dev_iommu_ops(dev);
bool has_pasid = msg->flags & IOMMU_PAGE_RESP_PASID_VALID;
-   struct iommu_domain *domain = iommu_get_domain_for_dev(dev);

-   if (!domain || !domain->ops->page_response)
+   if (!ops->page_response)
return -ENODEV;

if (!param || !param->fault_param)
@@ -1296,7 +1299,7 @@ int iommu_page_response(struct device *dev,
msg->pasid = 0;
}

-   ret = domain->ops->page_response(dev, evt, msg);
+   ret = ops->page_response(dev, evt, msg);
list_del(>list);
kfree(evt);
break;
@@ -1521,7 +1524,7 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group);

static int iommu_get_def_domain_type(struct device *dev)
{
-   const struct iommu_ops *ops = dev->bus->iommu_ops;
+   const struct iommu_ops *ops = dev_iommu_ops(dev);

if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
return IOMMU_DOMAIN_DMA;
@@ -1580,7 +1583,7 @@ static int iommu_alloc_default_domain(struct iommu_group 
*group,
 */
static struct iommu_group *iommu_group_get_for_dev(struct device *dev)
{
-   const struct iommu_ops *ops = dev->bus->iommu_ops;
+   const struct iommu_ops *ops = dev_iommu_ops(dev);
struct iommu_group *group;
int ret;

@@ -1588,9 +1591,6 @@ static struct iommu_group *iommu_group_get_for_dev(struct 
device *dev)
if (group)
return group;

-   if (!ops)
-   return ERR_PTR(-EINVAL);
-
group = ops->device_group(dev);
if (WARN_ON_ONCE(group == NULL))
return ERR_PTR(-EINVAL);
@@ -1759,10 +1759,10 @@ static int __iommu_group_dma_attach(struct iommu_group 
*group)

static int iommu_group_do_probe_finalize(struct device *dev, void *data)
{
-   struct iommu_domain *domain = data;
+   const struct iommu_ops *ops = dev_iommu_ops(dev);

-   if (domain->ops->probe_finalize)
-   domain->ops->probe_finalize(dev);
+   if (ops->probe_finalize)
+   ops->probe_finalize(dev);

return 0;
}
@@ -2020,7 +2020,7 @@