Re: [PATCH 1/2] iommu: free dev->iommu when iommu_fwspec_init() cannot allocate the fwspec

[Jason Gunthorpe <jgg@ziepe.ca>]

On Mon, Nov 02, 2026 at 04:51:05PM +0000, Robin Murphy wrote:
> On 31/10/2026 8:33 pm, Tal Zussman wrote:
> > iommu_fwspec_init() allocates dev->iommu with dev_iommu_get() and then
> > the fwspec. If the fwspec allocation fails it returns -ENOMEM but leaves
> > dev->iommu allocated, and it leaks.
> > 
> > kmemleak reports it when iommufd_fail_nth injects the allocation
> > failure:
> > 
> >    unreferenced object 0xffff88811c138b00 (size 192):
> >      comm "iommufd_fail_nt", pid 783
> >      backtrace:
> >        __kmalloc_cache_noprof
> >        dev_iommu_get
> >        iommu_fwspec_init
> >        iommu_mock_device_add
> >        iommufd_test
> >        iommufd_fops_ioctl
> > 
> > Free dev->iommu on failure if this call allocated it. iommu_init_device()
> > allocates it before calling into the driver and frees it on its own
> > error path, so leave it alone in that case.
> > 
> > Fixes: 72acd9df18f1 ("iommu: Move iommu_fwspec to struct dev_iommu")
> 
> No, the bug is clearly the early return in 2a918911ed3d ("iommufd: Register
> iommufd mock devices with fwspec"), so fix it there please.
> iommu_fwspec_init() has always been designed to be called in conjunction
> with iommu_probe_device() which handles cleanup appropriately.

Asking AI to audit this found the other case I remember...

Yes: ACPI DMA configuration during driver binding can leave dev->iommu
allocated after failure:

 really_probe()
  -> dev->bus->dma_configure()
    -> acpi_dma_configure_id()
      -> acpi_iommu_configure_id()
        -> IORT / RIMT / VIOT
          -> acpi_iommu_fwspec_init()
            -> iommu_fwspec_init()

 If dev_iommu_get() allocates successfully and the subsequent fwspec
 allocation fails, acpi_iommu_configure_id() (line 1614) does no
 cleanup. acpi_dma_configure_id() (line 1670) ignores errors other than
 -EPROBE_DEFER. This invocation has no enclosing iommu_init_device() to
 unwind it.

The OF path has its own manual free fix already, it doesn't rely on
iommu_probe_device() to clean it. The other driver users are the only
things actually protected by ops->probe_device().

> > @@ -3074,6 +3074,7 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode)
> >   {
> >   	const struct iommu_device *iommu = iommu_from_fwnode(iommu_fwnode);
> >   	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > +	bool new_iommu = !dev->iommu;
> >   	if (!iommu)
> >   		return driver_deferred_probe_check_state(dev);
> > @@ -3088,8 +3089,11 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode)
> >   	/* Preallocate for the overwhelmingly common case of 1 ID */
> >   	fwspec = kzalloc_flex(*fwspec, ids, 1);
> > -	if (!fwspec)
> > +	if (!fwspec) {
> > +		if (new_iommu)
> > +			dev_iommu_free(dev);
> >   		return -ENOMEM;
> > +	}

Just move this allocation before the dev_iommu_get() and free it if
the get fails. Then the function has a more sensible calling
convention that error means it doesn't need any unwinding. The
new_iommu is a little too weird.

Jason