Re: [PATCH v9 11/25] iommu/arm-smmu-v3-kvm: Add the kernel driver

[Mostafa Saleh <smostafa@google.com>]

On Thu, Oct 15, 2026 at 08:35:03PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] iommu/arm-smmu-v3: native driver fallback bypasses pKVM DMA isolation
> - [High] mm: UAF of smmu due to devres LIFO inversion on deferred probe
> - [Medium] iommu/arm-smmu-v3: aux device creation failure aborts initialization for remaining SMMUs
> --
> 
> commit 989c8869198ba4ebe30e4fc7554bb66910985d0b
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Add the kernel driver
>     
> When KVM runs in protected mode with CONFIG_ARM_SMMU_V3_PKVM, the
> hypervisor owns the SMMUv3 hardware and presents emulated SMMUs to the
> host through trap and emulate. This patch adds the host side of the driver,
> using an auxiliary device approach for the emulated SMMUs.
> 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-pkvm-init.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-pkvm-init.c
> > new file mode 100644
> > index 0000000000000..5c5f0ff455e3b
> > --- /dev/null
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-pkvm-init.c
> 
> [ ... ]
> 
> > +static int __init kvm_arm_smmu_v3_register(void)
> > +{
> > +	size_t nr_pages = smmu_hyp_pgt_pages();
> > +	int ret;
> > +
> > +	if (!is_protected_kvm_enabled() || !nr_pages)
> > +		return 0;
> > +
> > +	ret = kvm_arm_smmu_array_alloc();
> > +	if (ret)
> > +		goto out_err;
> > +
> > +	ret = platform_driver_probe(&smmuv3_nesting_driver, smmuv3_nesting_probe);
> > +	if (ret)
> > +		goto out_free;
> > +
> > +	ret = pkvm_iommu_register_driver(&kvm_nvhe_sym(smmu_ops), nr_pages);
> > +	if (ret)
> > +		goto out_unregister;
> > +
> > +	/*
> > +	 * These variables are stored in the nVHE image, and won't be accessible
> > +	 * after KVM initialization. Ownership of kvm_arm_smmu_array will be
> > +	 * transferred to the hypervisor as well.
> > +	 */
> > +	kvm_hyp_arm_smmu_v3_smmus = kvm_arm_smmu_array;
> > +	kvm_hyp_arm_smmu_v3_count = kvm_arm_smmu_cur;
> > +	return ret;
> > +
> > +out_unregister:
> > +	platform_driver_unregister(&smmuv3_nesting_driver);
> 
> [Severity: Critical]
> Does this fail-open behavior bypass pKVM DMA isolation?
> 
> If an SMMU is rejected by smmuv3_nesting_probe() (e.g. due to Cavium errata
> or unaligned MMIO), or if pkvm_iommu_register_driver() fails, the affected
> SMMU devices are left unbound or the nesting driver is unregistered.
> 
> Later, the native arm_smmu_driver is registered unconditionally in
> arm-smmu-v3.c and will successfully bind to these unbound SMMU devices.
> Since the hypervisor wasn't given the MMIO regions for the skipped SMMUs,
> it does not enforce stage-2 trapping on them. The host EL1 could then
> program these native SMMUs to perform unrestricted DMA into pKVM-protected
> guest or hypervisor memory, compromising protected VMs.
> 

Also by design as replied in v8:
https://lore.kernel.org/all/arKa-8QbSSQFrUbJ@google.com/

> > +out_free:
> > +	kvm_arm_smmu_array_free();
> > +out_err:
> > +	kvm_arm_smmu_count = 0;
> > +	kvm_arm_smmu_array = NULL;
> > +	return ret;
> > +}
> > +
> > +static int __init smmu_create_aux_device(struct device *dev, void *data)
> > +{
> > +	static int dev_id;
> > +	struct auxiliary_device *auxdev;
> > +
> > +	auxdev = __devm_auxiliary_device_create(dev, "protected_kvm",
> > +						"smmu_v3_emu", NULL, dev_id++);
> > +	if (!auxdev)
> > +		return -ENODEV;
> > +	return 0;
> > +}
> > +
> > +static int __init kvm_arm_smmu_v3_post_init(void)
> > +{
> > +	if (!kvm_arm_smmu_count)
> > +		return 0;
> > +
> > +	/*
> > +	 * If KVM failed to initialise, the host stage-2 is never enabled and
> > +	 * the SMMUs are driven natively through the same path.
> > +	 */
> > +	WARN_ON(driver_for_each_device(&smmuv3_nesting_driver.driver, NULL,
> > +				       NULL, smmu_create_aux_device));
> 
> [Severity: Medium]
> Does this halt initialization for all remaining SMMUs if one fails?
> 
> In kvm_arm_smmu_v3_post_init(), driver_for_each_device() iterates over all
> bound SMMUs. If smmu_create_aux_device() fails (e.g. due to an ID collision
> or memory allocation failure in __devm_auxiliary_device_create()), it
> returns -ENODEV.
> 
> The core driver_for_each_device() function halts iteration and returns
> immediately upon receiving a non-zero value. Consequently, if a single SMMU
> fails aux device creation, all subsequent SMMUs are silently skipped. These
> skipped SMMUs won't be probed by the host's arm_smmu_driver_emu, breaking
> DMA translation for any peripherals behind them.
> 
> Should the callback log the error and return 0 to allow the remaining
> devices to initialize?
> 

If anyone fails the system is not trusted so there is no point of
creating the others.

> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index c2ec33c763a0f..66f38c54742b8 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> 
> [ ... ]
> 
> > +static int arm_smmu_device_probe_emu(struct auxiliary_device *auxdev,
> > +				     const struct auxiliary_device_id *id)
> > +{
> > +	struct device *parent = auxdev->dev.parent;
> > +	void *group;
> > +	int ret;
> > +
> > +	dev_info(&auxdev->dev, "Probing from %s\n", dev_name(parent));
> > +	group = devres_open_group(parent, NULL, GFP_KERNEL);
> > +	if (!group)
> > +		return -ENOMEM;
> > +
> > +	ret = arm_smmu_device_probe(to_platform_device(parent));
> > +	if (ret)
> > +		devres_release_group(parent, group);
> > +	else
> > +		devres_close_group(parent, group);
> > +	return ret;
> > +}
> > +
> > +static void arm_smmu_device_remove_emu(struct auxiliary_device *auxdev)
> > +{
> > +	arm_smmu_device_remove(to_platform_device(auxdev->dev.parent));
> > +}
> 
> [Severity: High]
> Can this lead to a use-after-free of the smmu struct due to devres LIFO
> inversion during a deferred probe retry?
> 
> If arm_smmu_device_probe() returns -EPROBE_DEFER (e.g. from
> iommu_device_register() -> bus_iommu_probe() -> probe_iommu_group()),
> arm_smmu_device_probe_emu() correctly cleans up and returns the error,
> causing the auxiliary bus to defer the probe.
> 
> However, __devm_auxiliary_device_create() has already proceeded to register
> the auxiliary_device_destroy action on the parent's devres list. Later,
> when the deferred probe retries and succeeds, arm_smmu_device_probe()
> allocates the smmu struct via devm_kzalloc() and attaches it to the parent's
> devres list *after* the destroy action.
> 
> On parent platform device removal (e.g. via device tree overlay removal or
> system teardown), devres_release_all(parent) destroys devres actions in LIFO
> order. It first frees the smmu struct, and subsequently executes
> auxiliary_device_destroy. This unregisters the aux device and triggers
> arm_smmu_device_remove_emu(), which then dereferences the freed smmu pointer.

My assumption is that the platform device can't be removed as the
comment above, the overlay support has no in-kernel user nor a uAPI.

Not sure what does "system teardown" mean, but shutdown won't call
devres_release_all(). So I believe there is not path to remove the
platform device in that case.

However, as hardening, I might just call auxiliary_device_create()
instead of __devm_auxiliary_device_create().

Thanks,
Mostafa

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261015200903.2735565-1-smostafa@google.com?part=11