[PATCH] iommu: Guard iommu_group_first_dev() against an empty device list

[PATCH] iommu: Guard iommu_group_first_dev() against an empty device list

[Li Ximo]

From: Wang Haoran <haoranwangsec@gmail.com>

iommu_group_first_dev() calls list_first_entry(&group->devices, ...) and
immediately dereferences the result with ->dev, without checking whether
the group's device list is empty. list_first_entry() does not return NULL
on an empty list: it returns container_of() applied to the list head,
i.e. a non-NULL wild pointer pointing at &group->devices itself. Since
"list" is at offset 0 in struct group_device, the ->dev read then lands
on whatever struct iommu_group fields happen to follow the list head,
yielding a garbage struct device pointer that the callers
(__iommu_group_alloc_default_domain(), iommu_group_alloc_default_domain(),
__iommu_attach_group(), __iommu_group_alloc_blocking_domain()) go on to
dereference, e.g. via dev_iommu_ops() -> dev->iommu->ops, resulting in a
page fault / kernel oops. An empty group is a legitimate end state: the
list becomes empty when the group's last device is removed under
group->mutex, and nothing in the helper itself prevents it from being
called on such a group. Note that the sibling helper
iommu_group_for_each_dev() in the same file iterates the very same list
with the empty-safe list_for_each_entry(), which suggests the missing
guard here is an oversight rather than a deliberate design. This
potential issue was found during a code audit.

Fix this by making iommu_group_first_dev() return NULL when the device
list is empty - which is the semantically correct answer for "the group
has no devices" - and by teaching all four callers to handle NULL:
__iommu_group_alloc_default_domain() and iommu_group_alloc_default_domain()
return ERR_PTR(-ENODEV), while __iommu_attach_group() and
__iommu_group_alloc_blocking_domain() return -ENODEV, consistent with the
existing error handling in each function. Behaviour on non-empty groups
is unchanged.

Reported-by: Wang Haoran <haoranwangsec@gmail.com>
Reported-by: Li Ximo <lixicaicai@gmail.com>
Signed-off-by: Wang Haoran <haoranwangsec@gmail.com>
---
 drivers/iommu/iommu.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index cd1bca7ed..5f26785bb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1386,6 +1386,8 @@ EXPORT_SYMBOL_GPL(iommu_group_mutex_assert);
 static struct device *iommu_group_first_dev(struct iommu_group *group)
 {
 	lockdep_assert_held(&group->mutex);
+	if (list_empty(&group->devices))
+		return NULL;
 	return list_first_entry(&group->devices, struct group_device, list)->dev;
 }
 
@@ -1723,6 +1725,9 @@ __iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 	struct device *dev = iommu_group_first_dev(group);
 	struct iommu_domain *dom;
 
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+
 	if (group->default_domain && group->default_domain->type == req_type)
 		return group->default_domain;
 
@@ -1757,11 +1762,16 @@ __iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 static struct iommu_domain *
 iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 {
-	const struct iommu_ops *ops = dev_iommu_ops(iommu_group_first_dev(group));
+	struct device *dev = iommu_group_first_dev(group);
+	const struct iommu_ops *ops;
 	struct iommu_domain *dom;
 
 	lockdep_assert_held(&group->mutex);
 
+	if (!dev)
+		return ERR_PTR(-ENODEV);
+	ops = dev_iommu_ops(dev);
+
 	/*
 	 * Allow legacy drivers to specify the domain that will be the default
 	 * domain. This should always be either an IDENTITY/BLOCKED/PLATFORM
@@ -2362,6 +2372,8 @@ static int __iommu_attach_group(struct iommu_domain *domain,
 		return -EBUSY;
 
 	dev = iommu_group_first_dev(group);
+	if (!dev)
+		return -ENODEV;
 	if (!dev_has_iommu(dev) ||
 	    !domain_iommu_ops_compatible(dev_iommu_ops(dev), domain))
 		return -EINVAL;
@@ -3369,9 +3381,13 @@ void iommu_device_unuse_default_domain(struct device *dev)
 static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
 {
 	struct device *dev = iommu_group_first_dev(group);
-	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	const struct iommu_ops *ops;
 	struct iommu_domain *domain;
 
+	if (!dev)
+		return -ENODEV;
+	ops = dev_iommu_ops(dev);
+
 	if (group->blocking_domain)
 		return 0;
 
-- 
2.30.2