Re: [PATCH 1/2] iommu: Add iommu_is_available() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] iommu: iommu_available never cleared in iommu_device_unregister()
- [Medium] iommu: iommu_device_register() marks non-translating IOMMUs available
- [Low] iommu: unnecessary export and wrong doc for iommu_set_available()
- [Low] iommu: unmarked lockless access to iommu_available
--

commit 994e0def279fa954a8ab08056b43fb15157b3edd
Author: Mario Limonciello (AMD) <superm1@kernel.org>

iommu: Add iommu_is_available() API

This commit introduces iommu_is_available() and iommu_set_available() so
that security subsystems such as lockdown can query whether an IOMMU driver
has registered and probed on the platform.

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index b486b8bbd1fcd..d042944713ab4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -309,6 +309,13 @@ int iommu_device_register(struct iommu_device *iommu,
>  		iommu_device_unregister(iommu);
>  	else
>  		WRITE_ONCE(iommu->ready, true);
> +	/*
> +	 * Mark IOMMU as available if registration succeeded and bus probing
> +	 * succeeded. This enables security checks (e.g. lockdown IOMMU check)
> +	 * that depend on knowing whether hardware IOMMU isolation is present.
> +	 */
> +	if (!err)
> +		iommu_set_available();

[Severity: High]
What happens if all registered IOMMU devices are later unregistered via
iommu_device_unregister()?

Several IOMMU drivers perform additional fallible initialization steps after
calling iommu_device_register() and unwind with iommu_device_unregister() on
failure. For example, in drivers/iommu/amd/init.c:

iommu_init_pci()
  iommu_device_register()
  pci_enable_device()

and later in state_next():

state_next()
  amd_iommu_enable_interrupts()
  free_iommu_resources()
    free_iommu_all()
      free_iommu_one()
        free_sysfs()
          iommu_device_unregister(&iommu->iommu);

Similar post-registration error paths exist in mtk_iommu_probe() (when
component_master_add_with_match() fails) and sprd_iommu_probe() (when
sprd_iommu_clk_enable() or sprd_iommu_get_version() fails), as well as
driver unbind and hot-unplug paths.

Because iommu_device_unregister() removes the instance from
iommu_device_list without clearing or recalculating iommu_available when the
list becomes empty, won't iommu_is_available() permanently return true even
when no IOMMU device remains registered?

[Severity: Medium]
Could setting iommu_available unconditionally when iommu_device_register()
succeeds report true on platforms where no DMA translation or isolation is
actually active?

In drivers/iommu/iommu.c, probe_iommu_group() converts -ENODEV from
__iommu_probe_device() into 0, so bus_iommu_probe() succeeds even when an
IOMMU driver rejects all devices or provides no DMA translation:

1. In drivers/iommu/amd/init.c, iommu_init_pci() calls
   iommu_device_register() even when amd_iommu_pgtable == PD_MODE_NONE
   (where DMA translation is unsupported and only interrupt remapping is
   used, causing amd_iommu_probe_device() to return ERR_PTR(-ENODEV) for
   every device):

	ret = iommu_device_register(&iommu->iommu, &amd_iommu_ops, NULL);
	if (ret || amd_iommu_pgtable == PD_MODE_NONE) {
		iommu_device_sysfs_remove(&iommu->iommu);
	}

2. In drivers/xen/grant-dma-iommu.c, grant_dma_iommu_probe() registers
   grant_dma_iommu_ops, whose probe_device callback unconditionally returns
   ERR_PTR(-ENODEV) without providing hardware DMA isolation.

3. When default passthrough mode is active (iommu_default_passthrough() is
   true via iommu=pt, iommu.passthrough=1, or
   CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y), internal PCI/PCIe devices are placed
   in IOMMU_DOMAIN_IDENTITY domains with unrestricted DMA access to physical
   memory.

Should iommu_is_available() account for whether active DMA translation is
present rather than only checking if iommu_device_register() was called?

>  	return err;
>  }
>  EXPORT_SYMBOL_GPL(iommu_device_register);
> @@ -2961,6 +2968,47 @@ static int __init iommu_init(void)
>  }
>  core_initcall(iommu_init);
>  
> +static bool iommu_available __read_mostly;
> +
> +/**
> + * iommu_is_available() - Check if IOMMU subsystem is available

[ ... ]

> + */
> +bool iommu_is_available(void)
> +{
> +	return iommu_available;
> +}
> +EXPORT_SYMBOL_GPL(iommu_is_available);
> +
> +/**
> + * iommu_set_available() - Mark IOMMU as available
> + *
> + * Called by IOMMU drivers during probe to indicate that hardware IOMMU
> + * support is functional on this platform.
> + *
> + * Context: Called from driver probe paths.
> + */
> +void iommu_set_available(void)
> +{
> +	iommu_available = true;
> +}
> +EXPORT_SYMBOL_GPL(iommu_set_available);

[Severity: Low]
Since iommu_is_available() can be called at any time after module
initialization has begun while deferred or hotplug IOMMU driver probes may
call iommu_device_register() concurrently without holding a shared lock,
should the accesses to iommu_available use READ_ONCE() and WRITE_ONCE()
(similar to WRITE_ONCE(iommu->ready, true) in iommu_device_register()), or
check !list_empty(&iommu_device_list) under iommu_device_lock, to avoid
KCSAN data-race warnings?

[Severity: Low]
Is there any reason to declare iommu_set_available() in
include/linux/iommu.h and export it via EXPORT_SYMBOL_GPL()?

The kernel-doc comment states that it is called by IOMMU drivers during
probe, but its only caller is iommu_device_register() in
drivers/iommu/iommu.c. Could iommu_set_available() be made static (or folded
directly into iommu_device_register()) so arbitrary modules cannot flip this
security-sensitive state without registering an IOMMU device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-lockdown-iommu-v1-0-00f3c2be4c9f@kernel.org?part=1