iommu/vt-d: Use-after-free on sva unbind device with pending IOPFs

[Ethan MILON <ethan.milon@eviden.com>]

Hi,

While testing SVA in QEMU with Intel IOMMU, I encountered a double free in
the kernel when unbinding a device with page requests still in flight. This
results in a use-after-free error and eventually a kernel panic:

[   48.227596] refcount_t: underflow; use-after-free.
[   48.227949] WARNING: CPU: 3 PID: 313 at lib/refcount.c:28 refcount_warn_saturate+0xd8/0xe0
[   48.228531] Modules linked in: sva(OE) xfs(E) sr_mod(E) sd_mod(E) cdrom(E) sg(E) ahci(E) libahci(E) libata(E) e1000(E) dm_mod(E)
[   48.229460] CPU: 3 UID: 0 PID: 313 Comm: kworker/u16:4 Kdump: loaded Tainted: G           OE       6.16.0-rc5+ #59 PREEMPT(voluntary)
[   48.230231] Tainted: [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
[   48.230568] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[   48.231278] Workqueue: iopf_queue/dmar0-iopfq iommu_sva_handle_iopf
[   48.231697] RIP: 0010:refcount_warn_saturate+0xd8/0xe0
[   48.232028] Code: ff 48 c7 c7 58 eb 2e 82 c6 05 40 2f 35 01 01 e8 ae e8 ab ff 0f 0b c3 48 c7 c7 00 eb 2e 82 c6 05 2c 2f 35 01 01 e8 98 e8 ab ff <0f> 0b c3 0f 1f 44 00 003
[   48.233202] RSP: 0018:ffffc900008afe50 EFLAGS: 00010286
[   48.233541] RAX: 0000000000000000 RBX: ffff888113dccb40 RCX: 0000000000000000
[   48.234007] RDX: ffff888861daa480 RSI: 0000000000000001 RDI: ffff888861d9c140
[   48.234461] RBP: ffff8881118e0000 R08: 0000000000000000 R09: ffffc900008afcf8
[   48.234942] R10: ffffc900008afcf0 R11: ffffffff829dfde8 R12: ffff888100f5c000
[   48.235396] R13: ffff888100051c00 R14: ffff888100f5c005 R15: ffff88810c480000
[   48.235860] FS:  0000000000000000(0000) GS:ffff8888dea4f000(0000) knlGS:0000000000000000
[   48.236381] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   48.236763] CR2: 0000558328aa6468 CR3: 0000000111332002 CR4: 0000000000770ef0
[   48.237222] PKRU: 55555554
[   48.237397] Call Trace:
[   48.237561]  <TASK>
[   48.237727]  iopf_free_group+0xe/0x20
[   48.237978]  process_one_work+0x197/0x3d0
[   48.238239]  worker_thread+0x23a/0x350
[   48.238487]  ? rescuer_thread+0x4a0/0x4a0
[   48.238760]  kthread+0xf8/0x230
[   48.238972]  ? finish_task_switch.isra.0+0x81/0x260
[   48.239290]  ? kthreads_online_cpu+0x110/0x110
[   48.239598]  ? kthreads_online_cpu+0x110/0x110
[   48.239899]  ret_from_fork+0x13b/0x170
[   48.240155]  ? kthreads_online_cpu+0x110/0x110
[   48.240434]  ret_from_fork_asm+0x11/0x20
[   48.240711]  </TASK>
[   48.240864] ---[ end trace 0000000000000000 ]---

The trace show that it's caused by a double free of the same iopf group :

irq/24-d-66        2..... : iommu_report_device_fault <-prq_event_thread
irq/24-d-66        2..... : iopf_get_dev_fault_param.isra.0 <-iommu_report_device_fault
irq/24-d-66        2..... : iopf_group_alloc <-iommu_report_device_fault
irq/24-d-66        2..... : iommu_sva_iopf_handler <-iommu_report_device_fault
    test-1633      0..... : iommu_sva_unbind_device <-test_handle.isra.0
    test-1633      0..... : iommu_detach_device_pasid <-iommu_sva_unbind_device
    test-1633      0..... : iopf_queue_remove_device <-blocking_domain_set_dev_pasid
    test-1633      0..... : iopf_free_group <-iopf_queue_remove_device
    test-1633      0..... : iopf_put_dev_fault_param <-iopf_free_group
    test-1633      0..... : iopf_put_dev_fault_param <-iopf_queue_remove_device
    test-1633      0..... : iommu_domain_free <-iommu_sva_unbind_device
kworker/-313       3..... : iommu_sva_handle_iopf <-process_one_work
kworker/-313       3..... : iopf_group_response <-iommu_sva_handle_iopf
kworker/-313       3..... : iopf_free_group <-process_one_work
kworker/-313       3..... : iopf_put_dev_fault_param <-iopf_free_group

Unbinding a device while page requests are still in flight is likely an edge 
case that shouldn't occur, but investigating it has led me to two questions:

1. The issue appear at the commit 17fce9d2336d ("iommu/vt-d: Put iopf
enablement in domain attach path"), where device are now dynamically added and
removed from the iopf queue. This commit add a call to the new
iopf_for_domain_remove() function when removing pasid :

> @@ -4041,6 +4053,7 @@ static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
>  {
>         struct device_domain_info *info = dev_iommu_priv_get(dev);
> 
> +       iopf_for_domain_remove(old, dev);
>         intel_pasid_tear_down_entry(info->iommu, dev, pasid, false);
>         domain_remove_dev_pasid(old, dev, pasid);
>
>         return 0;
>  }

Moving iopf_for_domain_remove() after domain_remove_dev_pasid() fix the issue
above.

Shouldn't iopf_for_domain_remove() be called after
intel_pasid_tear_down_entry() or domain_remove_dev_pasid() to ensure the device
can no longer send page requests and the queue has been flushed? Is the current
ordering intentional?

2. Regarding commit 9759ae2cee7c ("iommu: Fix potential memory leak in
iopf_queue_remove_device()"): the commit frees the pending IOPF groups, but in
the scenario above, direct references to the groups still exists either in the
handler or in the workqueue awaiting processing, leading to a double free.
Reverting this commit fix the issue above.

I am not sure to see where is the potential memory leak, is there a case where
a reference to a group exists only in the iommu_fault_param::faults that I'm
missing?

Thanks,
Ethan