[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

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

[Jason Gunthorpe]

On Fri, Oct 23, 2026 at 02:57:05PM +0800, Li Ximo wrote:
> 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;

list_first_entry_or_null()

>  }
>  
> @@ -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);
> +

Are any of these even possible to be called with an empty group? How?

The giant wall of text at the top failed to explain how this could
ever even happen.

Jason