Re: [PATCH V4 7/9] x86/hyperv: Implement Hyper-V virtual IOMMU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Partial IOMMU unmaps corrupt the interval tree state and silently leak DMA mappings in the hypervisor, allowing potential DMA use-after-free.
- [High] `hv_iommu_domain_alloc_paging` unconditionally rejects domain allocation if the calling process is not an active VMM, breaking VFIO for host applications.
- [High] The `hv_iommu_map_pages` callback ignores the caller-provided `gfp` flags and implicitly uses `GFP_KERNEL`, risking sleep-in-atomic panics.
- [High] Unsafe interval tree iteration during node removal breaks RB-tree invariants and causes memory corruption or skipped nodes.
- [Medium] The `pci_devs_to_skip` string parsing logic incorrectly aborts on a successful parse, completely breaking the `hv_iommu_skip=` boot parameter.
- [Medium] `hv_iommu_domain_free` does not free remaining `hv_iommu_mapping` nodes in the `mappings_tree`, leading to a memory leak.
- [Medium] Failure in `hv_iommu_attach_dev` leaves the hardware detached while failing to restore the device to its previous domain.
--

--- Patch [7]: [PATCH V4 7/9] x86/hyperv: Implement Hyper-V virtual IOMMU ---
commit eedf8d68fd442834b8bd7d597c6c152d43c81395
Author: Mukesh R <mrathor@linux.microsoft.com>
Subject: x86/hyperv: Implement Hyper-V virtual IOMMU

This commit adds a new virtual IOMMU driver for Microsoft Hyper-V. The
hypervisor claims the IOMMU upon boot, and this driver communicates with
it to create and delete paging domains, map and unmap pages, and handle
default identity domains.

> diff --git a/drivers/iommu/hyperv/hv-iommu-root.c b/drivers/iommu/hyperv/hv-iommu-root.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/iommu/hyperv/hv-iommu-root.c

[ ... ]

> +static struct iommu_domain *hv_iommu_domain_alloc_paging(struct device *dev)
> +{
> +	struct hv_domain *hvdom;
> +	int rc;
> +	u32 unique_id;
> +	u64 ptid = hv_get_current_partid();
> +
> +	if (ptid == HV_PARTITION_ID_INVALID)
> +		return NULL;

[Severity: High]
The commit message explicitly mentions support for host apps like SPDK, but
could this check unintentionally reject domain allocation for them? 

Since host applications like SPDK are standard userspace processes rather
than mshv VMM threads, hv_get_current_partid() will return
HV_PARTITION_ID_INVALID, which blocks the allocation entirely.

[ ... ]

> +static void hv_iommu_domain_free(struct iommu_domain *immdom)
> +{
> +	unsigned long flags;
> +	u64 status;
> +	struct hv_input_delete_device_domain *input;
> +	struct hv_input_device_domain *ddp;
> +	struct hv_domain *hvdom = to_hv_domain(immdom);
> +
> +	if (hv_special_domain(hvdom))
> +		return;
> +
> +	local_irq_save(flags);
> +	input = *this_cpu_ptr(hyperv_pcpu_input_arg);
> +	ddp = &input->device_domain;
> +	memset(input, 0, sizeof(*input));
> +
> +	ddp->partition_id = HV_PARTITION_ID_SELF;
> +	ddp->domain_id.type = HV_DEVICE_DOMAIN_TYPE_S2;
> +	ddp->domain_id.id = hvdom->domid_num;
> +
> +	status = hv_do_hypercall(HVCALL_DELETE_DEVICE_DOMAIN, input,
> +				 NULL);
> +	local_irq_restore(flags);
> +
> +	if (!hv_result_success(status))
> +		hv_status_err(status, "\n");
> +
> +	kfree(hvdom);
> +}

[Severity: Medium]
When an IOMMU domain is destroyed, does this leave the dynamically allocated
nodes inside hvdom->mappings_tree leaked? 

The function frees the hvdom structure but there does not appear to be a
cleanup loop iterating over and freeing the struct hv_iommu_mapping objects
left in the tree.

[ ... ]

> +static int hv_iommu_attach_dev(struct iommu_domain *immdom, struct device *dev,
> +			       struct iommu_domain *old)
> +{
> +	struct pci_dev *pdev;
> +	int rc;
> +	struct hv_domain *hvdom_new = to_hv_domain(immdom);
> +	struct hv_domain *hvdom_prev = to_hv_domain(old);
> +
> +	/* Only allow PCI devices for now */
> +	if (!dev_is_pci(dev))
> +		return -EINVAL;
> +
> +	pdev = to_pci_dev(dev);
> +
> +	/* There are no explicit detach calls, hence check first if we need
> +	 * to detach first. Also, in case of guest shutdown, it's the VMM
> +	 * thread that attaches it back to the hv_def_identity_dom, and
> +	 * hvdom_prev will not be null then. It is null during boot.
> +	 */
> +	if (hvdom_prev && !hv_special_domain(hvdom_prev))
> +		hv_iommu_detach_dev(hvdom_prev, dev);
> +
> +	rc = hv_iommu_att_dev2dom(hvdom_new, pdev);
> +
> +	if (rc == 0)
> +		dev_iommu_priv_set(dev, hvdom_new);  /* sets "private" field */
> +	else
> +		dev_iommu_priv_set(dev, NULL);
> +
> +	return rc;
> +}

[Severity: Medium]
If the device successfully detaches from the old domain but the new attach
hypercall fails, is the hardware left in limbo? 

Since there is no rollback to re-attach the device to the previous domain,
the subsystem logic might believe the device is safely detached and handle
the error, while the hardware is left without an assigned domain.

[ ... ]

> +static int hv_iommu_add_tree_mapping(struct hv_domain *hvdom,
> +				     unsigned long iova, phys_addr_t paddr,
> +				     size_t size, u32 flags)
> +{
> +	unsigned long irqflags;
> +	struct hv_iommu_mapping *mapping;
> +
> +	mapping = kzalloc_obj(struct hv_iommu_mapping);
> +	if (!mapping)
> +		return -ENOMEM;

[Severity: High]
Can this allocation trigger a panic by sleeping in atomic context? 

The kzalloc_obj macro implicitly uses a GFP_KERNEL allocation by default.
The caller, hv_iommu_map_pages(), receives a gfp_t parameter specifically
because DMA mapping operations can be called with GFP_ATOMIC inside
interrupt handlers, but that parameter is discarded before calling this
function.

[ ... ]

> +static size_t hv_iommu_del_tree_mappings(struct hv_domain *hvdom,
> +					unsigned long iova, size_t size)
> +{
> +	unsigned long flags;
> +	size_t unmapped = 0;
> +	unsigned long last = iova + size - 1;
> +	struct hv_iommu_mapping *mapping = NULL;
> +	struct interval_tree_node *node, *next;
> +
> +	spin_lock_irqsave(&hvdom->mappings_lock, flags);
> +	next = interval_tree_iter_first(&hvdom->mappings_tree, iova, last);
> +	while (next) {
> +		node = next;
> +		mapping = container_of(node, struct hv_iommu_mapping, iova);
> +		next = interval_tree_iter_next(node, iova, last);

[Severity: High]
Is it safe to use a cached next pointer for interval tree iteration after
modifying the tree? 

Calling interval_tree_remove() later in this loop restructures the
underlying RB-tree and changes parent and child relationships. Continuing
the iteration with a pre-cached pointer on the mutated tree might break the
iterator's traversal logic, leading to skipped nodes or memory corruption.

> +
> +		/* Trying to split a mapping? Not supported for now. */
> +		if (mapping->iova.start < iova)
> +			break;
> +
> +		unmapped += mapping->iova.last - mapping->iova.start + 1;
> +
> +		interval_tree_remove(node, &hvdom->mappings_tree);
> +		kfree(mapping);
> +	}
> +	spin_lock_irqrestore(&hvdom->mappings_lock, flags);
> +
> +	return unmapped;
> +}

[Severity: High]
What happens if a caller unmaps a subset of a mapping? 

If the unmap starts exactly at the IOVA, hv_iommu_del_tree_mappings()
removes the entire node from the tree and returns the node's full size.
Then, hv_iommu_unmap_pages() only unmaps the originally requested size
from the hypervisor. 

This appears to permanently lose the remainder of the mapping from the
tree while leaving it mapped in the hypervisor, causing a desync and
potential DMA memory leak.

[ ... ]

> +static struct iommu_device *hv_iommu_probe_device(struct device *dev)
> +{
> +	if (!dev_is_pci(dev))
> +		return ERR_PTR(-ENODEV);
> +
> +	if (pci_devs_to_skip && *pci_devs_to_skip) {
> +		int rc, pos = 0;
> +		int parsed;
> +		int segment, bus, slot, func;
> +		struct pci_dev *pdev = to_pci_dev(dev);
> +
> +		do {
> +			parsed = 0;
> +
> +			rc = sscanf(pci_devs_to_skip + pos, " (%x:%x:%x.%x) %n",
> +				    &segment, &bus, &slot, &func, &parsed);
> +			if (rc)
> +				break;

[Severity: Medium]
Does this condition inadvertently terminate the loop on a successful parse?

The sscanf function returns the number of successfully matched items. If
the configuration string is parsed correctly, rc will be greater than zero
and the loop will immediately break before checking the parsed fields or
matching the devices to skip.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718021949.926306-1-mrathor@linux.microsoft.com?part=7