Re: [PATCH V4 8/9] mshv: Populate mmio mappings for PCI passthru

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

New issues:
- [Critical] hv_map_mmio_pages extrapolates the physical base address for an entire MMIO region from a single page fault, allowing unauthorized arbitrary physical memory access.
- [High] Flawed alignment logic allows mapping unaligned addresses as 2M large pages and fails to iterate the physical frame variable.
- [High] Unconditional use of FAULT_FLAG_WRITE in mshv_chk_get_mmio_start_pfn breaks read-only MMIO mappings.
- [High] Missing cleanup of partial mappings in hv_map_mmio_pages on failure.
- [High] Resolving user space PFNs without synchronizing with MMU notifiers introduces a TOCTOU race.

Pre-existing issues:
- [Critical] Guest MMIO mappings are never torn down when host userspace unmaps the VMA or detaches the device.
- [High] Synchronous, unbounded loop over page_is_ram for the entire region size can cause soft lockups.
- [High] Error handling rollback in hv_do_map_gpa_hcall fails to unmap pages due to a mismatched page count unit.
--

--- Patch [8]: [PATCH V4 8/9] mshv: Populate mmio mappings for PCI passthru ---
commit 65da49ecfde6e092de4c941dffc7d00deae59b78
Author: Mukesh R <mrathor@linux.microsoft.com>

mshv: Populate mmio mappings for PCI passthru

This commit handles unmapped GPA intercepts by resolving the PCI resource
pfn for a guest gpa and dynamically asking the hypervisor to map it via
hypercalls. It attempts to use 2M page sizes where possible and removes
the old vma->vm_pgoff mapping approach.

> diff --git a/drivers/hv/mshv_root_hv_call.c b/drivers/hv/mshv_root_hv_call.c
> --- a/drivers/hv/mshv_root_hv_call.c
> +++ b/drivers/hv/mshv_root_hv_call.c
[ ... ]
> @@ -189,31 +189,33 @@ int hv_call_delete_partition(u64 partition_id)
>  }
>  
>  /* Ask the hypervisor to map guest ram pages or the guest mmio space */
> -static int hv_do_map_gpa_hcall(u64 partition_id, u64 gfn, u64 page_struct_count,
> -			       u32 flags, struct page **pages, u64 mmio_spa)
> +static int hv_do_map_gpa_hcall(u64 partition_id, u64 gfn, u64 page_count,
> +			       u32 flags, struct page **pages, u64 mmio_mfn)
>  {
>  	struct hv_input_map_gpa_pages *input_page;
>  	u64 status, *pfnlist;
>  	unsigned long irq_flags, large_shift = 0;
> -	int ret = 0, done = 0;
> -	u64 page_count = page_struct_count;
> -
> -	if (page_count == 0 || (pages && mmio_spa))
> -		return -EINVAL;
> +	int i, ret = 0, done = 0;
> +	u64 adj_page_count = page_count;
>  
> -	if (flags & HV_MAP_GPA_LARGE_PAGE) {
> -		if (mmio_spa)
> +	if (mmio_mfn) {
> +		if (pages)
>  			return -EINVAL;
> +		for (i = 0; i < page_count; i++)
> +			if (page_is_ram(mmio_mfn + i))
> +				return -EINVAL;

[Severity: High]
This is a pre-existing issue, but could this synchronous loop cause soft
lockups?

If a user maps a multi-gigabyte MMIO region (like a large GPU BAR), this
loops page_count times calling page_is_ram() synchronously for every 4KB
page without preemption, which could trigger kernel soft lockup watchdogs.

[ ... ]
>  		} else if (!hv_result_success(status)) {
> +			pr_err("%s: failed to map pages at gfn %#llx: completed %u/%llu, flags=%#x, status=%#llx (%s)\n",
> +			       __func__, gfn, done, page_count, flags, status,
> +			       hv_result_to_string(hv_result(status)));
>  			ret = hv_result_to_errno(status);
>  			break;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does the error rollback at
the end of hv_do_map_gpa_hcall() pass the wrong unit to
hv_call_unmap_gpa_pages()?

If an error occurs during mapping and HV_MAP_GPA_LARGE_PAGE is set, "done"
contains the number of mapped 2M pages. This is passed to
hv_call_unmap_gpa_pages(), which expects a 4K page count:

hv_do_map_gpa_hcall() {
    ...
    if (ret && done) {
        u32 unmap_flags = 0;
        if (flags & HV_MAP_GPA_LARGE_PAGE)
            unmap_flags |= HV_UNMAP_GPA_LARGE_PAGE;
        hv_call_unmap_gpa_pages(partition_id, gfn, done, unmap_flags);
    }
}

Since hv_call_unmap_gpa_pages() expects 4K pages, it evaluates
!HV_PAGE_COUNT_2M_ALIGNED(done), which fails for small integers and aborts
with -EINVAL, leaking the memory mappings in the hypervisor's page tables.

[ ... ]
> -/* Ask the hypervisor to map guest mmio space */
> -int hv_call_map_mmio_pages(u64 partition_id, u64 gfn, u64 mmio_spa, u64 numpgs)
> +/*
> + * Ask the hypervisor to map guest mmio space. Don't set HV_MAP_GPA_NOT_CACHED
> + * in hcall flags for slightly better performance, and in that case the guest
> + * stage 1 page table will control caching.
> + */
> +int hv_map_mmio_pages(u64 partition_id, struct mshv_mem_region *reg, u64 gfn,
> +		      u64 mmio_mfn)
>  {
> -	int i;
> -	u32 flags = HV_MAP_GPA_READABLE | HV_MAP_GPA_WRITABLE |
> -		    HV_MAP_GPA_NOT_CACHED;
> +	int rc;
> +	u32 flags = HV_MAP_GPA_READABLE;
> +	u64 hpages, numpgs, numpgs_in_hpage = HPAGE_SIZE / PAGE_SIZE;
> +
> +	if (reg->hv_map_flags & HV_MAP_GPA_WRITABLE)
> +		flags |= HV_MAP_GPA_WRITABLE;
> +	if (reg->hv_map_flags & HV_MAP_GPA_EXECUTABLE)
> +		flags |= HV_MAP_GPA_EXECUTABLE;
> +
> +	mmio_mfn = mmio_mfn - (gfn - reg->start_gfn);	/* start of the range */

[Severity: Critical]
Does extrapolating the physical base address from a single page fault allow
mapping arbitrary physical memory?

If a user creates a large MSHV region backed by a sparse or smaller VM_PFNMAP
VMA, mshv_handle_unmapped_gpa() resolves a single PFN. This extrapolation then
calculates the base PFN for the entire region and maps reg->nr_pages of
contiguous physical memory into the guest. This would map the holes or
unrelated physical memory (like other devices' MSI-X tables or kernel RAM),
bypassing VFIO's memory protections.

> +
> +	numpgs = 0;
> +	gfn = reg->start_gfn;
> +	while (!HV_PAGE_COUNT_2M_ALIGNED(gfn) &&
> +	       !HV_PAGE_COUNT_2M_ALIGNED(mmio_mfn) &&
> +	       numpgs < reg->nr_pages) {
> +		numpgs++;
> +		gfn++;
> +	}

[Severity: High]
Does this loop condition prematurely exit, and are we failing to iterate
mmio_mfn?

If gfn and mmio_mfn have different 2M offsets, the logical AND causes the loop
to exit as soon as one of them is aligned. Furthermore, mmio_mfn is never
incremented inside this loop, which leaves it misaligned when it is later
passed to the large page mapping request.

> +
> +	if (numpgs) {
> +		rc = hv_do_map_gpa_hcall(partition_id, reg->start_gfn, numpgs,
> +					 flags, NULL, mmio_mfn);
> +		if (rc || numpgs == reg->nr_pages)
> +			return rc;
> +	}
> +
> +	mmio_mfn = mmio_mfn + numpgs;
> +	numpgs = reg->nr_pages - numpgs;
> +
> +	if (numpgs < numpgs_in_hpage)
> +		return hv_do_map_gpa_hcall(partition_id, gfn, numpgs, flags,
> +					   NULL, mmio_mfn);
> +
> +	for (hpages = 0; numpgs >= numpgs_in_hpage;) {
> +		hpages++;
> +		numpgs = numpgs - numpgs_in_hpage;
> +	}
> +	rc = hv_do_map_gpa_hcall(partition_id, gfn, hpages * numpgs_in_hpage,
> +				 flags | HV_MAP_GPA_LARGE_PAGE, NULL, mmio_mfn);
> +	if (rc)
> +		return rc;

[Severity: High]
Are we leaking partial mappings here if the hypercall batch fails?

If the initial batch of 4K pages succeeds but this second batch for 2M pages
fails, the function returns the error code directly. This skips cleaning up
the pages mapped during the first hv_do_map_gpa_hcall(), leaking those
mappings in the hypervisor.

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
[ ... ]
> +static int mshv_chk_get_mmio_start_pfn(u64 uaddr, u64 *mmio_pfnp)
> +{
> +	struct vm_area_struct *vma;
> +	bool is_mmio;
> +	struct follow_pfnmap_args pfnmap_args;
> +	int rc = -EINVAL;
> +
> +	mmap_read_lock(current->mm);
> +	vma = vma_lookup(current->mm, uaddr);
> +	is_mmio = vma ? !!(vma->vm_flags & (VM_IO | VM_PFNMAP)) : 0;
> +	if (!is_mmio)
> +		goto unlock_mmap_out;
> +
> +	pfnmap_args.vma = vma;
> +	pfnmap_args.address = uaddr;
> +
> +	rc = follow_pfnmap_start(&pfnmap_args);
> +	if (rc) {
> +		rc = fixup_user_fault(current->mm, uaddr, FAULT_FLAG_WRITE,
> +				      NULL);

[Severity: High]
Does this unconditional use of FAULT_FLAG_WRITE break read-only MMIO
mappings?

When resolving an unmapped GPA intercept for a read-only VMA, the
fixup_user_fault() call will fail with -EFAULT because the VMA lacks write
permissions, completely denying the guest access to read-only MMIO regions.

> +		if (rc)
> +			goto unlock_mmap_out;
> +
> +		rc = follow_pfnmap_start(&pfnmap_args);
> +		if (rc)
> +			goto unlock_mmap_out;
> +	}
> +
> +	*mmio_pfnp = pfnmap_args.pfn;
> +	follow_pfnmap_end(&pfnmap_args);
> +
> +unlock_mmap_out:
> +	mmap_read_unlock(current->mm);
> +	return rc;
> +}
> +
> +/*
> + * Check if the unmapped gpa belongs to mmio space. If yes, resolve it.
> + *
> + * Returns: True if valid mmio intercept and handled, else false.
> + */
> +static bool mshv_handle_unmapped_gpa(struct mshv_vp *vp)
> +{
[ ... ]
> +	rc = mshv_chk_get_mmio_start_pfn(uaddr, &mmio_mfn);
> +	if (rc)
> +		goto put_rg_out;
> +
> +	rc = hv_map_mmio_pages(pt->pt_id, rg, gfn, mmio_mfn);

[Severity: High]
Is there a TOCTOU race here when mapping the resolved PFN?

mshv_chk_get_mmio_start_pfn() resolves the PFN and drops mmap_read_lock
before returning. There is no synchronization with the MMU interval notifier
sequence lock prior to calling hv_map_mmio_pages(). If the VMA is unmapped
concurrently after the lock is dropped, could the hypervisor map a stale or
freed PFN into the guest?

[ ... ]
> @@ -1376,11 +1449,7 @@ mshv_map_user_memory(struct mshv_partition *partition,
>  					    region->nr_pages,
>  					    HV_MAP_GPA_NO_ACCESS, NULL);
>  		break;
> -	case MSHV_REGION_TYPE_MMIO:
> -		ret = hv_call_map_mmio_pages(partition->pt_id,
> -					     region->start_gfn,
> -					     mmio_pfn,
> -					     region->nr_pages);
> +	default:
>  		break;
>  	}

[Severity: Critical]
This is a pre-existing issue, but are guest MMIO mappings ever torn down
when host userspace unmaps the VMA or detaches the device?

Because MMIO regions fall into the default case, they skip
mshv_prepare_pinned_region() and do not register MMU interval notifiers.
When the host userspace calls munmap() on the VFIO region or the device is
detached, the guest's stage-2 page tables are never invalidated, leaving the
guest with arbitrary access to reassigned physical memory.

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