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

[Robin Murphy <robin.murphy@arm.com>]

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