linux-kernel - Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL blocking

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4037d353-0ec4-4f68-a09c-564b93ba313b@kernel.org>
Date: Mon, 29 Sep 2025 15:30:22 +0200
From: Hans Verkuil <hverkuil+cisco@...nel.org>
To: Jason Gunthorpe <jgg@...dia.com>
Cc: iommu@...ts.linux.dev, Linux Kernel <linux-kernel@...r.kernel.org>,
 Linux Media Mailing List <linux-media@...r.kernel.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH] iommu: __iommu_attach_group: check for non-NULL
 blocking_domain

On 29/09/2025 15:02, Jason Gunthorpe wrote:
> On Mon, Sep 29, 2025 at 02:18:50PM +0200, Hans Verkuil wrote:
>> On 29/09/2025 14:07, Jason Gunthorpe wrote:
>>> On Mon, Sep 29, 2025 at 10:23:47AM +0200, Hans Verkuil wrote:
>>>
>>>> Since I am unfamiliar with the iommu core code, I am uncertain whether I am
>>>> just papering over a bug elsewhere, or whether this is really the correct solution.
>>>
>>> It is papering over something, group->domain is not supposed to be
>>> NULL at this point.. That probably means the iommu driver has not been
>>
>> It's group->blocking_domain that's NULL, not group->domain.
> 
> Er, I thought you were hitting a false positive on this:
> 
>   group->domain != group->blocking_domain
> 
> ie NULL != NULL
> 
> But I suppose the whole expression is checking for group->domain
> already.
> 
> All your patch does is entirely disable the safetly logic :\
> 
> What is isp_attach_iommu() trying to accomplish? It does
> arm_iommu_detach_device() and then arm_iommu_attach_device() ?
> 
> Why?
> 
> Is this trying to force a non-identity translation for ISP?

I have absolutely no idea. The commit where this was added is this:

---------------------------------------------------------
commit 2a0a5472af5caa0d0df334abb9975dc496f045da
Author: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Date:   Thu Jan 2 20:06:08 2014 -0300

[media] omap3isp: Use the ARM DMA IOMMU-aware operations

Attach an ARM DMA I/O virtual address space to the ISP device. This
switches to the IOMMU-aware ARM DMA backend, we can thus remove the
explicit calls to the OMAP IOMMU map and unmap functions.

Signed-off-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Acked-by: Sakari Ailus <sakari.ailus@....fi>
Signed-off-by: Mauro Carvalho Chehab <m.chehab@...sung.com>
---------------------------------------------------------


That's over 11 years ago. I've CC-ed Sakari in case he remembers something about
this.

Later this was simplified a bit:

---------------------------------------------------------
commit fd8e2d4b393252505783656471465c7f85f3c0a9
Author: Suman Anna <s-anna@...com>
Date:   Wed Apr 12 00:21:32 2017 -0500

omap3isp: Remove iommu_group related code

The OMAP IOMMU driver has added the support for IOMMU groups internally,
and the ISP device is automatically linked to the appropriate IOMMU group.
So, remove the explicit function calls that creates/deletes an iommu_group
and adds the ISP device to this group.
---------------------------------------------------------


And finally the code detaching the device (which you referred to in your
reply) was added here (much more recently):

---------------------------------------------------------
commit 6bc076eec6f85f778f33a8242b438e1bd9fcdd59
Author: Robin Murphy <robin.murphy@....com>
Date:   Mon Oct 28 17:58:36 2024 +0000

media: omap3isp: Handle ARM dma_iommu_mapping

It's no longer practical for the OMAP IOMMU driver to trick
arm_setup_iommu_dma_ops() into ignoring its presence, so let's use the
same tactic as other IOMMU API users on 32-bit ARM and explicitly kick
the arch code's dma_iommu_mapping out of the way to avoid problems.
---------------------------------------------------------


All I know is that something is wrong after blocking_domain was added, which
now causes the "failed to create ARM IOMMU mapping" error from isp.c when the
omap3isp driver is probed.

My (very likely flawed) reasoning for this patch was that if there is no
blocking_domain (i.e. it's a NULL pointer), then the group->domain should
just be accepted. But that reasoning was just based on the field names, and with
no actual understanding of what is going on here :-)

Regards,

	Hans