Re: [PATCH v1] iommu/hyperv: Don't free a paging domain the hypervisor did not delete

[Jacob Pan]

Hi Yuho,

On Thu,  5 Nov 2026 14:27:08 -0500
Yuho Choi <oss.patchbox@xxxxxxxxx> wrote:

> 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.
> 
> Fixes: 2690d4156b51 ("iommu/hyperv: Add para-virtualized IOMMU
> support for Hyper-V guest") Signed-off-by: Yuho Choi
> <oss.patchbox@xxxxxxxxx> ---
> 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(-)
> 
> diff --git a/drivers/iommu/hyperv/hv-iommu-guest.c
> b/drivers/iommu/hyperv/hv-iommu-guest.c index
> 876a22f21f1e..205c642c8d40 100644 ---
> a/drivers/iommu/hyperv/hv-iommu-guest.c +++
> b/drivers/iommu/hyperv/hv-iommu-guest.c @@ -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);
>  }
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;
}