Re: [PATCH v1] iommu/vsi: Fix use-after-free during module unload

[Benjamin Gaignard <benjamin.gaignard@collabora.com>]

Le 05/09/2026 à 20:38, Yibo Tan a écrit :
> iommu_device_register() links the embedded iommu_device into the IOMMU
> core's global device list. The VSI driver can be built as a module, but
> has no remove callback to unregister the device before devres frees the
> containing struct vsi_iommu.
>
> With no attached consumer holding a module reference, unloading
> vsi-iommu.ko succeeds. A later platform device registration enters the
> IOMMU bus notifier and scans the stale list entry. KASAN reports a
> slab-use-after-free in __iommu_probe_device().
>
> The missing unregister operation on driver unbind was also noted during
> review of the driver's fwnode lookup lifetime handling.
>
> Add the missing remove callback. Unregister the IOMMU device and remove
> its sysfs object while the provider is still alive. Release the exact
> shared IRQ action before forcing runtime suspend, then unprepare the
> clocks acquired during probe.
>
> The failure was reproduced on the 2026-08-11 IOMMU next snapshot with
> real module load and unload syscalls. With the same KASAN kernel, the
> unmodified driver produced two invalid reads from the same freed list
> entry. The patched driver removed the entry, completed the later device
> registration and produced no KASAN, WARNING, Oops or panic.
>
> The remove callback also builds with W=1 for arm64 with
> ARCH_ROCKCHIP=y and CONFIG_PM=y, and for the arm64 COMPILE_TEST path
> with CONFIG_PM=n.
>
> A standalone reproducer, the vulnerable and fixed serial logs, and
> their checksums are available at:
>
> https://github.com/kimaiden1984-boop/linux-vsi-iommu-unload-uaf-reproducer
>
> Fixes: 917ace84b770 ("iommu: Add verisilicon IOMMU driver")
> Link: https://lore.kernel.org/0e405cb3-1227-4ad2-96ff-aa0db3124381@arm.com/
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:GPT-5
> Signed-off-by: Yibo Tan <lhfff@tju.edu.cn>
> ---
> The QEMU helper supplies the platform device, MMIO resource, IRQ and
> firmware node normally provided by RK3588 hardware. The failing access
> occurs while the IOMMU core scans its provider list, before VSI register
> access.
>
> Not tested on physical RK3588 hardware: removal with an attached
> decoder, a runtime-active device, or concurrent interrupt delivery.
>
>   drivers/iommu/vsi-iommu.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/drivers/iommu/vsi-iommu.c b/drivers/iommu/vsi-iommu.c
> index 42c424496..7fa7ee8cf 100644
> --- a/drivers/iommu/vsi-iommu.c
> +++ b/drivers/iommu/vsi-iommu.c
> @@ -728,6 +728,17 @@ static int vsi_iommu_probe(struct platform_device *pdev)
>   	return err;
>   }
>   
> +static void vsi_iommu_remove(struct platform_device *pdev)
> +{
> +	struct vsi_iommu *iommu = platform_get_drvdata(pdev);
> +
> +	iommu_device_unregister(&iommu->iommu);
> +	iommu_device_sysfs_remove(&iommu->iommu);
> +	devm_free_irq(&pdev->dev, iommu->irq, iommu);

I don't think devm_free_irq() is needed here.
That said we need to fix the problem.
Why not introduce devm_ functions for iommu_device_unregister(), iommu_device_sysfs_remove() and
clk_bulk_unprepare() ? That could be useful for lot of drivers.

Regards,
Benjamin

> +	pm_runtime_force_suspend(&pdev->dev);
> +	clk_bulk_unprepare(iommu->num_clocks, iommu->clocks);
> +}
> +
>   static void vsi_iommu_shutdown(struct platform_device *pdev)
>   {
>   	struct vsi_iommu *iommu = platform_get_drvdata(pdev);
> @@ -776,6 +787,7 @@ static DEFINE_RUNTIME_DEV_PM_OPS(vsi_iommu_pm_ops,
>   
>   static struct platform_driver rockchip_vsi_iommu_driver = {
>   	.probe = vsi_iommu_probe,
> +	.remove = vsi_iommu_remove,
>   	.shutdown = vsi_iommu_shutdown,
>   	.driver = {
>   		   .name = "vsi_iommu",