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

[Mostafa Saleh <smostafa@google.com>]

On Tue, Sep 22, 2026 at 01:28:14PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] iommu: pKVM DMA isolation bypass via unbound SMMUs falling back to host driver
> - [High] iommu: Use-after-free of smmu during platform_device removal
> --
> 
> commit a8ae8dbe858fbefdcb8b149d519f07175f91d794
> Author: Mostafa Saleh <smostafa@google.com>
> 
> iommu/arm-smmu-v3-kvm: Add the kernel driver
> 
> This commit introduces the pKVM host driver for the Arm SMMUv3, allowing
> the hypervisor to trap and emulate SMMU management when KVM runs in
> protected mode. The driver probes these SMMUs and creates auxiliary
> devices for them on the aux bus.
> 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-kvm.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-kvm.c
> > new file mode 100644
> > index 0000000000000..9947d3a44304b
> > --- /dev/null
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-kvm.c
> [ ... ]
> > +static int smmuv3_nesting_probe(struct platform_device *pdev)
> > +{
> > +	struct hyp_arm_smmu_v3_device *smmu = &kvm_arm_smmu_array[kvm_arm_smmu_cur];
> > +	struct device *dev = &pdev->dev;
> > +	struct resource *res;
> > +
> > +	/* Only device tree, ACPI not supported. */
> > +	if (!dev->of_node)
> > +		return -EINVAL;
> 
> [Severity: Critical]
> Can an adversary bypass pKVM stage-2 memory protections here?
> 
> If an SMMU fails to bind to the hypervisor due to early returns in this
> probe function (like missing a device tree node or hitting the cavium
> erratum below), it is excluded from kvm_arm_smmu_array and the hypervisor
> does not trap its MMIO.
> 
> When the native host arm_smmu_driver registers at device_initcall, it can
> successfully bind to this unbound SMMU. Does this allow an adversary host
> kernel at EL1 to natively program this untrapped SMMU to perform arbitrary
> DMA into hypervisor or guest memory?

Yes, nothing really we can do about this, the system might have:
- Incompatible SMMUv3 instance.
- Device behind another IOMMUs.
- Devices without IOMMUs.

A system shipping this code should ensure that the driver is probed
successfully on their HW.

> 
> > +
> > +	if (kvm_arm_smmu_cur >= kvm_arm_smmu_count)
> > +		return -ENOSPC;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res)
> > +		return -ENODEV;
> > +
> > +	if (of_property_read_bool(dev->of_node, "cavium,cn9900-broken-page1-regspace"))
> > +		return -EINVAL;
> [ ... ]
> > 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 2043c6dc1bdf8..d6d1e2d30ef63 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));
> 
> [Severity: High]
> Can this devres setup lead to a use-after-free during platform device
> removal?
> 
> If the parent physical platform device is destroyed (for example, via
> dynamic DT overlay removal), the driver core calls devres_release_all()
> which processes cleanups in reverse order of addition (LIFO).
> 
> Because arm_smmu_device_probe() adds the smmu allocation to the parent's
> devres list after the auxiliary device was created in
> smmu_create_aux_device(), the smmu memory gets freed first.

Two points:
1) At the moment the device can't be released as suppress_bind_attrs is
   true. There is no kernel user for the dynamic overlay it is added
   for testing AFAIU.

2) AFAICT, __devm_auxiliary_device_create() calls, devm_add_action_or_reset()
   for the device destroy, so the aux device will destroyed before the
   platform device.

> 
> Following that, the auxiliary_device_destroy devres action executes,
> unregistering the aux device and triggering driver unbind via
> arm_smmu_device_remove_emu() and arm_smmu_device_remove(). Does this
> result in dereferencing the already freed smmu struct?
> 

Same as above.

Thanks,
Mostafa

> > +	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));
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260922131259.2975334-1-smostafa@google.com?part=11