iommu/hyperv: Don't free a paging domain the hypervisor did not delete
Subsystems (3)
PATCH 1/1 iommu/hyperv: Don't free a paging domain the hypervisor did not delete
hv_iommu_paging_domain_free() frees all page-table pages with pt_iommu_deinit() and only then asks the hypervisor to delete the device domain that points at them. If HVCALL_DELETE_DEVICE_DOMAIN fails, hv_delete_device_domain() only logs the error, returns the domain ID to the IDA, and the domain is freed anyway.
The hypervisor then keeps a device domain whose page tables have been freed and can be reused for anything. The ID is also handed out again by the next paging domain allocation, while the hypervisor still has a domain with that ID.
Delete the domain before freeing its page tables, and keep the ID, the page tables and the domain if the hypervisor fails to delete it.
---
Compile-tested only (x86_64 with HYPERV and HYPERV_PVIOMMU, W=1); I have no Hyper-V guest with the para-virtualized IOMMU to test it on.
drivers/iommu/hyperv/hv-iommu-guest.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
drivers/iommu/hyperv/hv-iommu-guest.c +13 -4
@@ -107,7 +107,7 @@ static int hv_create_device_domain(struct hv_iommu_domain *hv_domain)return hv_result_to_errno (status); } -static void hv_delete_device_domain (struct hv_iommu_domain *hv_domain) +static int hv_delete_device_domain (struct hv_iommu_domain *hv_domain) {u64 status;unsigned long flags;
@@ -122,11 +122,15 @@ static void hv_delete_device_domain(struct hv_iommu_domain *hv_domain)local_irq_restore (flags); -if (!hv_result_success (status)) +if (!hv_result_success (status)) { +/* The domain still exists, so its ID cannot be reused */ hv_status_err (status,"HVCALL_DELETE_DEVICE_DOMAIN failed\n" ); +return hv_result_to_errno (status); + }ida_free (&hv_domain->hv_iommu->domain_ids, hv_domain->device_domain.domain_id.id); +return 0 ; } static int
@@ -466,11 +470,16 @@ static void hv_iommu_paging_domain_free(struct iommu_domain *domain) {struct hv_iommu_domain *hv_domain =to_hv_iommu_domain (domain); +/* +* If the hypervisor still has the domain, it may still use its page +* tables, so leak them. +*/ +if (hv_delete_device_domain (hv_domain)) +return ; +/* Free all remaining mappings */ pt_iommu_deinit (&hv_domain->pt_iommu); -hv_delete_device_domain (hv_domain); -kfree (hv_domain); }
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
Signature · 1 line
2.43.0
Quoted 16 lines · Yuho Choi
hv_iommu_paging_domain_free()frees all page-table pages withpt_iommu_deinit()and only then asks the hypervisor to delete the device domain that points at them. IfHVCALL_DELETE_DEVICE_DOMAINfails,hv_delete_device_domain()only logs the error, returns the domain ID to the IDA, and the domain is freed anyway.The hypervisor then keeps a device domain whose page tables have been freed and can be reused for anything. The ID is also handed out again by the next paging domain allocation, while the hypervisor still has a domain with that ID.
Delete the domain before freeing its page tables, and keep the ID, the page tables and the domain if the hypervisor fails to delete it.
Fixes 2690d4156b51 ("iommu/hyperv: Add para-virtualized IOMMU support for Hyper-V guest")Signed-off-by Yuho Choi <[email protected]>
Quoted 8 lines
---
Compile-tested only (x86_64with HYPERV andHYPERV_PVIOMMU, W=1); I have no Hyper-V guest with the para-virtualized IOMMU to test it on.drivers/iommu/hyperv/hv-iommu-guest.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
Good catch!
Quoted 5 lines · Yuho Choi
hv_iommu_paging_domain_free()frees all page-table pages withpt_iommu_deinit()and only then asks the hypervisor to delete the device domain that points at them. IfHVCALL_DELETE_DEVICE_DOMAINfails,hv_delete_device_domain()only logs the error, returns the domain ID to the IDA, and the domain is freed anyway.
Isn't there a WARN_ON on this path? I recall asking for one ..
So long as that is there this is the right way to continue
Hi Yuho,
Quoted 64 lines · On Thu, 5 Nov 2026 14:27:08 -0500 Yuho Choi <[email protected]> wrote:
hv_iommu_paging_domain_free()frees all page-table pages withpt_iommu_deinit()and only then asks the hypervisor to delete the device domain that points at them. IfHVCALL_DELETE_DEVICE_DOMAINfails,hv_delete_device_domain()only logs the error, returns the domain ID to the IDA, and the domain is freed anyway.The hypervisor then keeps a device domain whose page tables have been freed and can be reused for anything. The ID is also handed out again by the next paging domain allocation, while the hypervisor still has a domain with that ID.
Delete the domain before freeing its page tables, and keep the ID, the page tables and the domain if the hypervisor fails to delete it.
Fixes 2690d4156b51 ("iommu/hyperv: Add para-virtualized IOMMUsupport for Hyper-V guest") Signed-off-by: Yuho Choi <[email protected]> ---
Compile-tested only (x86_64with HYPERV andHYPERV_PVIOMMU, W=1); I have no Hyper-V guest with the para-virtualized IOMMU to test it on.drivers/iommu/hyperv/hv-iommu-guest.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)(quoted diff) +0
-0
b/drivers/iommu/hyperv/hv-iommu-guest.cindex876a22f21f1e..205c642c8d40100644 ---a/drivers/iommu/hyperv/hv-iommu-guest.c+++b/drivers/iommu/hyperv/hv-iommu-guest.c@@ -107,7 +107,7 @@ static inthv_create_device_domain(struct hv_iommu_domain *hv_domain)returnhv_result_to_errno(status); }-static void
hv_delete_device_domain(struct hv_iommu_domain *hv_domain)+static inthv_delete_device_domain(struct hv_iommu_domain *hv_domain){drivers/iommu/hyperv/hv-iommu-guest.c +2
-1u64 status;unsigned long flags;@@ -122,11 +122,15 @@ static void hv_delete_device_domain(struct hv_iommu_domain *hv_domain)local_irq_restore (flags ); -if (!hv_result_success (status)) +if (!hv_result_success (status)) { +/* The domain still exists, so its ID cannot be reused */
hv_status_err(status, "HVCALL_DELETE_DEVICE_DOMAIN failed\n");drivers/iommu/hyperv/hv-iommu-guest.c +5
-0+return hv_result_to_errno (status); + }ida_free (&hv_domain->hv_iommu->domain_ids, hv_domain->device_domain.domain_id.id); +return 0 ; } static int@@ -466,11 +470,16 @@ static void hv_iommu_paging_domain_free(struct iommu_domain *domain) {struct hv_iommu_domain *hv_domain =to_hv_iommu_domain (domain); +/* +* If the hypervisor still has the domain, it may still use its page
drivers/iommu/hyperv/hv-iommu-guest.c +2
-0+* tables, so leak them. +*/
drivers/iommu/hyperv/hv-iommu-guest.c static void hv_iommu_paging_domain_free(struct iommu_domain *domain) · line 484
Quoted 10 lines
drivers/iommu/hyperv/hv-iommu-guest.c +3
-2+if (hv_delete_device_domain (hv_domain)) +return ; +/* Free all remaining mappings */ pt_iommu_deinit (&hv_domain->pt_iommu); -hv_delete_device_domain (hv_domain); -kfree (hv_domain); }
Looks good but I think it is incomplete.
if both hv_configure_device_domain and hv_delete_device_domain both fails in hv_create_configure_device_domain, then unwind code will do pt_iommu_deinit thus have the same UAF problem.
ret = hv_create_configure_device_domain(hv_domain, &settings);
if (ret)
goto err_pt_deinit;
return &hv_domain->domain;err_pt_deinit:
pt_iommu_deinit(&hv_domain->pt_iommu);
hv_create_configure_device_domain(struct hv_iommu_domain *hv_domain, const struct hv_device_domain_settings *settings)
{
int ret;
ret = hv_create_device_domain(hv_domain);
if (ret)
return ret;
ret = hv_configure_device_domain(hv_domain, settings);
if (ret)
hv_delete_device_domain(hv_domain);//fails here
return ret;
}Quoted 7 lines · Yuho Choi
Quoted 5 lines
hv_iommu_paging_domain_free()frees all page-table pages withpt_iommu_deinit()and only then asks the hypervisor to delete the device domain that points at them. IfHVCALL_DELETE_DEVICE_DOMAINfails,hv_delete_device_domain()only logs the error, returns the domain ID to the IDA, and the domain is freed anyway.Isn't there a
WARN_ONon this path? I recall asking for one ..
There isn't one yet. hv_delete_device_domain() only prints the error with hv_status_err(). The driver does WARN when a flush fails or when attaching to the blocking domain fails, but not when a delete fails.
I'll add a WARN for the delete failure in v2.
Quoted 1 line
Reviewed-by Jason Gunthorpe <[email protected]>
Thanks for the review!
Quoted 5 lines
Looks good but I think it is incomplete.
if both
hv_configure_device_domainandhv_delete_device_domainboth fails inhv_create_configure_device_domain, then unwind code will dopt_iommu_deinitthus have the same UAF problem.
You're right, thanks for catching this. If the delete also fails there, hv_iommu_domain_alloc_paging() still frees the page tables.
In v2, I'll make hv_iommu_domain_alloc_paging() call hv_create_device_domain() and hv_configure_device_domain() itself, so it can leak the page tables and the domain when the delete fails, the same way hv_iommu_paging_domain_free() does.