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

[Robin Murphy]

On 2026-11-03 7:32 pm, Jason Gunthorpe wrote:
> 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.

Yes, because that path will no longer allocate - if we reach 
iommu_fwspec_init() from the really_probe path at all now it's because 
the IOMMU is not yet registered, thus we'll always take the early return.

In fact I think the only reason to still have dev_iommu_get() in 
iommu_fwspec_init(), rather than just dereferencing dev->iommu directly, 
is for the sake of those few remaining drivers which abuse 
of_dma_configure() directly that I'm not sure what to do about.

Thanks,
Robin.
> 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