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@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Reported-by: Li Ximo <lixicaicai@xxxxxxxxx>
> Signed-off-by: Wang Haoran <haoranwangsec@xxxxxxxxx>
> ---
>  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